@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.