daniellansun commented on code in PR #3242: URL: https://github.com/apache/groovy/pull/3242#discussion_r4513076345
########## src/main/java/org/codehaus/groovy/tools/javac/MemClassOutputClasspath.java: ########## @@ -0,0 +1,152 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.codehaus.groovy.tools.javac; + +import groovy.lang.GroovyClassLoader; + +import java.io.ByteArrayInputStream; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.InputStream; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLConnection; +import java.net.URLDecoder; +import java.net.URLStreamHandler; +import java.nio.charset.StandardCharsets; +import java.util.Collections; +import java.util.Map; +import java.util.Objects; +import java.util.UUID; +import java.util.WeakHashMap; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Publishes in-memory class bytes through a {@link GroovyClassLoader} so + * joint compilation can resolve them without a class-output directory. + * <p> + * The protocol is not registered with the JVM. Only URLs created here can + * be opened, and each URL serves just the map it was given. Paths that + * escape that map are rejected. No filesystem or network access is performed. + */ +final class MemClassOutputClasspath { + + private static final String PROTOCOL = "x-groovy-mem"; + + private static final Map<GroovyClassLoader, Map<String, byte[]>> INSTALLED = + Collections.synchronizedMap(new WeakHashMap<>()); + + private MemClassOutputClasspath() { + } + + /** + * Attaches an in-memory classpath to {@code loader}, or returns the map + * already attached to that loader. A second compile on the same loader + * therefore replaces entries in place and stays visible to + * {@code getResource}. + * + * @param loader the compilation unit's class loader + * @return the mutable resource map, keyed by {@code com/example/Type.class} + */ + @SuppressWarnings("deprecation") // custom handler; URL.of(URI, handler) is not on the Java 17 baseline + static Map<String, byte[]> install(final GroovyClassLoader loader) { Review Comment: I looked at the callers again instead of defending the test. `groovyc` (`FileSystemCompiler`) does `new JavaAwareCompilationUnit(configuration)`, which builds a new `GroovyClassLoader` for that unit. The Ant task does the same in `Groovyc#buildClassLoaderFor`: a new `GroovyClassLoader` on every run, then one unit. Neither reuses a loader across units. `GroovyClassLoader#parseClass` does reuse the outer loader, but not in the way the map assumed. `createCompilationUnit` builds a plain `CompilationUnit` on that loader, not a `JavaAwareCompilationUnit`. The classes are defined on a new `InnerLoader` per `parseClass`. That inner loader delegates `getResource` and `addURL` to the outer loader. Joint compilation never goes through this path, so the map was not installed for it. The one production place that builds a new `JavaAwareCompilationUnit` on an existing loader is groovy-lsp (`GroovyCompiler#loaderFor`). It reuses the loader while compiler settings are unchanged. It sets `memStub` and still writes javac output to a temp directory. It does not set `memClassOutput`, and a typical recompile overlaps the previous one. So there is no current caller for which a loader-to-store cache is the thing that makes `memClassOutput` work. The test `testSecondCompileOnTheSameLoaderSeesTheNewJavaApi` was the situation. I removed the cache and that test. A recompile of the same classes in this PR is a new `GroovyClassLoader`, which is what those entry points already do. The same unit can still `compile` again, including after `compile(Phases.SEMANTIC_ANALYSIS)` and after `addSources`, because the store is held by the unit. ########## src/main/java/org/codehaus/groovy/tools/javac/MemClassOutputClasspath.java: ########## @@ -0,0 +1,152 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.codehaus.groovy.tools.javac; + +import groovy.lang.GroovyClassLoader; + +import java.io.ByteArrayInputStream; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.InputStream; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLConnection; +import java.net.URLDecoder; +import java.net.URLStreamHandler; +import java.nio.charset.StandardCharsets; +import java.util.Collections; +import java.util.Map; +import java.util.Objects; +import java.util.UUID; +import java.util.WeakHashMap; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Publishes in-memory class bytes through a {@link GroovyClassLoader} so + * joint compilation can resolve them without a class-output directory. + * <p> + * The protocol is not registered with the JVM. Only URLs created here can + * be opened, and each URL serves just the map it was given. Paths that + * escape that map are rejected. No filesystem or network access is performed. + */ +final class MemClassOutputClasspath { + + private static final String PROTOCOL = "x-groovy-mem"; + + private static final Map<GroovyClassLoader, Map<String, byte[]>> INSTALLED = + Collections.synchronizedMap(new WeakHashMap<>()); + + private MemClassOutputClasspath() { + } + + /** + * Attaches an in-memory classpath to {@code loader}, or returns the map + * already attached to that loader. A second compile on the same loader + * therefore replaces entries in place and stays visible to + * {@code getResource}. + * + * @param loader the compilation unit's class loader + * @return the mutable resource map, keyed by {@code com/example/Type.class} + */ + @SuppressWarnings("deprecation") // custom handler; URL.of(URI, handler) is not on the Java 17 baseline + static Map<String, byte[]> install(final GroovyClassLoader loader) { Review Comment: Agreed. I took the map out of this PR. A second unit on the same loader only does something useful when the class names do not overlap, or when nothing has been defined yet. I could not find a normal API that is that situation. `parseClass` is the long-lived loader you described, and it already uses a short-lived `InnerLoader` for the definitions. A map keyed by the compilation unit's loader does not follow that split: `parseClass` does not build a `JavaAwareCompilationUnit`, and `InnerLoader#addURL` would publish on the parent anyway. The language server does reuse a loader across units, but it recompiles overlapping files and does not use in-memory class output. Caching on that loader would be the wrong structure even there. What remains is the part this issue is actually about. With `memStub` and `memClassOutput`, javac `CLASS_OUTPUT` stays in memory for that compilation unit, the unit's loader can resolve it, and no stub directory or target directory is required. The store lives on the unit. A failed compile drops the stage. A later unit does not share it. Compiling the same classes again is a new loader. I would like to file the cross-unit question separately once this is in, and look at it against `parseClass` / a real embedder rather than a test that passes the same loader into two units. The outline I have in mind: - Title along the lines of "GROOVY-12711 follow-up: in-memory class output across compilation units". - Show, through an existing API, which loader is long-lived and which loader defines classes. - Respect that a URL cannot be removed and that a defined class cannot be replaced, so appending another mem URL on a long-lived loader is not an update mechanism. - Do not turn `memClassOutput` on inside `parseClass` or the language server as part of that issue; those are consumers of whatever rule we settle. No such cache is in this PR anymore. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
