@bot-gradle test RFM

OK, I've already triggered ReadyForMerge build for you.

@big-guy At first sight, I can't see how that cache breaks the reason for removing it in the first place. But I may be missing something.
Any opinion?

@ljacomet I'm +1 for fixing this. I should have added a perf test when I made this switch before.

I'm -1 with fixing it in this way. There are a few problems:

• The cache isn't limited in any way or cleaned up. Workers are reused across subprojects, so this could impact large multi-project builds.
• The cache is based on the file paths and not the contents of the classpath. The classpath can be made up of changing jars with identical paths (although, most of the time it isn't).
• We already cache most of the worker's implementation, so caching the ZincScalaCompiler is masking something else.
• I'm not sure we can safely reuse ZincScalaCompiler due to the way we use AnalysisStoreProvider. There's certainly some state that would get carried across subprojects now.

I believe the actual problem is this classloader is not reused/cached:

By caching the ZincScalaCompiler, we also reuse this classloader. If you change that code to...

...
    static ClassLoader scalaClassLoader;

    private static ScalaInstance getScalaInstance(final Iterable<File> scalaClasspath) {
        if (scalaClassLoader == null) {
            scalaClassLoader = getClassLoader(scalaClasspath);
        }
...

I see a similar level of improvement. This is obviously the wrong kind of fix too, but I think it demonstrates that reusing that classloader has some impact. I suspect this may be something that the old zinc compiler was doing for us since we didn't worry about caching these things before.

With 6.4-rc-2: https://e.grdev.net/s/cceccmm6swwxg/performance/execution 38 minutes of task execution
With naive fix above: https://e.grdev.net/s/wficag6rkd4iu/performance/execution 21 minutes of task execution

(sorry, these are private links)

I think a proper fix would be to limit the change to just caching this classloader. It would be nice if we had access to some of the infrastructure we have for this (e.g., HashingClassLoaderFactory and ClassLoaderCache), but those aren't available in the worker and getting those services into the worker is a bigger task.

Maybe we can do something a little simpler:

• Instead of sending a Set<File> to ZincScalaCompilerFactory#getCompiler, let's send a ClassPath and HashCode.
• On the daemon side (where we have all the services we need), calculate the HashCode for the Scala ClassPath with ClasspathHasher in ScalaCompilerFactory and DefaultScalaToolProvider to pass through to ZincScalaCompilerFactory.
• In ZincScalaCompilerFactory, use a Guava cache with weak values or a fixed size to cache HashCode -> ClassLoader.

WDYT?

I agree with all your points. I think I can prepare a better fix according to this guidelines. Unless someone else wants to tackle the issue.

@SheliakLyr If you are willing to keep working on this, please do so. Otherwise I can't guarantee it will be done in the short term.
Thanks a lot for your work so far!

I've just pushed a new commit.

Currently, this branch contains "FixV2". Unfortunately performance is still worse than 5.6.x, but compared to 6.x improvement is significant! Keep in mind that my test-project is specifically designed to showcase the problem.

In branch cachedScalaCompilerV4 you can see an additional improvement:

private static final ClassLoaderCache ANALYZING_COMPILER_CLASSLOADER_CACHE =
   new ClassLoaderCache(new URLClassLoader(new URL[0]));
/// ...
if (scalaCompiler instanceof AnalyzingCompiler) {
            scalaCompiler = ((AnalyzingCompiler) scalaCompiler).withClassLoaderCache(ANALYZING_COMPILER_CLASSLOADER_CACHE);
}

This is something I found while trying to speed up the compilation even further. Seems a little risky to include, as I haven't found any documentation about this cache usage.

IMO, FixV2 should be good for now. Further optimizations can be be added later.

Cc @retronym in case he has thoughts.

As you've found, sbt.internal.inc.AnalyzingCompiler contains a classLoaderCache: ClassloaderCache field. Successive instantiations of AnalyzingCompiler can share an instance of this cache.

In the context of Zinc, this cache is used to re-use the classloader containing the Scala Instance JARs and the compiler bridge JAR. Cache entries are invalidated using timestamps (not content hashes), and are retained via a soft-reference without any time-based eviction.

Depending on which factory methods you are using, it might be simpler to get the cache from the first compiler you create and then call withClassLoaderCache to assign that to subsequent instances.

Here's where SBT uses a process-wide instance of that cache when instantiating Zinc. It seems to use some non-standard factories. I'm not sure why.

The ClassLoaderCache should be closed() when disposing it your compiler wrappers to close cached classloaders. This releases locks on files on Windows and allows the JVM to unload classes.

There may be a resource leak there in ClassPathCache's Map[... SoftReference[ClassLoader]]. The cache perhaps should have a ReferenceQueue of classloader that GC wants to evict, and call close() on them. That would save waiting for finalizers to close files represented by File objects that the classloader references. Maybe in practice GC will get rid of the unreachable ClassLoader and the File-s that it references at the same time though.

/cc @dwijnand @eed3si9n @eatkins

More context on how sbt manages its ClassLoaderCache is available here: sbt/sbt@03bf539. The basic idea is that we want there to be one global ClassLoaderCache to minimize the classloading overhead and reduce the metaspace footprint of sbt. What I think is different from gradle is that sbt also evaluates run and test in process and the global ClassLoaderCache is also used for the classloaders used by those tasks but, if possible, it recycles layers that were used for compilation. As @retronym suggested, the sbt implementation of ClassLoaderCache has a ReferenceQueue of ClassLoader and a reaper thread to close those loaders that are no longer referenced.

Thank you all for the hints. If I understand correctly, I can use the ClassLoaderCache for different AnalysisCompiler instances. This seems to be the reason for it's existence.

After reading your comments, I understood that my current cache is already buggy: uses weak references instead of soft references (that's trivial) and does not close the constructed classloaders. Default implementation of a ClassLoaderCache (actually AbstractClassLoaderCache) is also leaky.

My idea right now is to create a simple implementation of classloader cache based on guava. I have 3 variants:

1. Llimited size + strong references + close on eviction. Very simple, but risky if we can't get the max size right. On the other hand, those caches do not need to be very big, as I do not expect a lot of misses. Gradle worker is session-bound so this caches will be dropped after a single run of the build tool.
2. Limited size + soft references + reference queue + cleanup on get() operation. In this case, guava cache will be of type Cache[Key, ClassLoaderReference] where ClassLoaderReference is a subtype of SoftReference. This solution tries to avoid an additional thread, which unfortunately might be wrong. Cleaning reference queue on get() operations will effectively keep entire classloader in memory (ClassLoaderReference must keep a strong reference to Closeable classloader and soft reference for just a delegating object). In the case of a memory pressure, soft values will be released, but this will still keep classloader in memory until get() is called.
3. Limited size + soft references + reference queue + reaper thread. Requires additional threads :(

Currently, I have an experimental version of the 2nd approach on branch cachedScalaCompilerV5 . Speedup is significant (~28s). Personally however, I think that (1) should do the job. We can use very small cache sizes (5). Typical project will probably use the same classloader for all modules.

Guava'a soft/weak values cannot be used, as they lack a proper mechanism for resource release.

For ClassLoaderCache, as a key, I use a pair (filename, timestamp). This is not a gradle-way, but consistent with the way sbt works and @big-guy noticed that the hashing infrastructure is not available in the worker.

After some thinking, I decided to use the 1st approach from my previous post. Meaning no soft values, caches are limited only by a number of entries. This is less cumbersome and I personally don't have a very good experience with soft references (or rather... the way they impact gc).

I have just force-pushed a new version. Please review & tell me what you think.

This is great news. We updated Apache Kafka to Gradle 6.3 (and later Gradle 6.4) and there have been multiple complaints regarding compilation speed. So, I'm encouraged by this development.

This is now merged, thanks again!

Please let others know so that we get testing with 6.5 RC1 which should be out early next week.

Thanks @ljacomet! I am really glad you merged this already. Eagerly awaiting RC1.

Looks like the milestone associated should be updated from 6.6 RC1 to 6.5 RC1.

🤦 Fixed.
Looks like in my eagerness to include in 6.5, I forgot to update both the issue and the PR milestones.