Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider using ClassLoaderCache in Scala's integration #4744

Closed
jvican opened this issue Jul 14, 2017 · 3 comments
Closed

Consider using ClassLoaderCache in Scala's integration #4744

jvican opened this issue Jul 14, 2017 · 3 comments

Comments

@jvican
Copy link

jvican commented Jul 14, 2017

/cc @stuhood
When I was having a look at the Scala Zinc integration, I've noticed that you don't use the ClassLoaderCache when instantiating Scala's incremental compiler:

  def newScalaCompiler(instance: XScalaInstance, interfaceJar: File): AnalyzingCompiler =
    new AnalyzingCompiler(
      instance,
      CompilerBridgeProvider.constant(interfaceJar, instance),
      ClasspathOptionsUtil.auto,
      _ => (), None
    )

instead of what our default Zinc utils do:

  def scalaCompiler(
      scalaInstance: xsbti.compile.ScalaInstance,
      compilerBridgeJar: File,
      classpathOptions: ClasspathOptions
  ): AnalyzingCompiler = {
    val bridgeProvider = constantBridgeProvider(scalaInstance, compilerBridgeJar)
    val emptyHandler = (_: Seq[String]) => ()
    val loader = Some(new ClassLoaderCache(new URLClassLoader(Array())))
    new AnalyzingCompiler(
      scalaInstance,
      bridgeProvider,
      classpathOptions,
      emptyHandler,
      loader
    )
  }

Note the use of Some instead of None.

Using the classloader cache can provide great speedups, as evidenced by sbt/sbt#2754.

Is this an oversight or have you tried this option in the past and didn't happen to improve performance for Pants?

@stuhood
Copy link
Member

stuhood commented Jul 14, 2017

Will give it a shot. Relates to #4477

@stuhood stuhood added the jvm label Jul 14, 2017
@stuhood
Copy link
Member

stuhood commented Jul 14, 2017

@jvican : Is it still useful entirely within one run? Or is it supposed to be used across multiple AnalyzingCompiler instances?

@jvican
Copy link
Author

jvican commented Jul 14, 2017

This is supposed to be used across multiple AnalyzingCompiler instances. I think it's under that scenario where performance improves considerably.

stuhood pushed a commit that referenced this issue Aug 11, 2017
### Problem

While working through #4477, it became obvious that:
1. the performance achieved when the analysis cache is missed is pretty abysmal
2. a `log4j` JMX error was being logged during startup
3. the `forkJava` and `javaHome` settings are redundant: the only reason to pass `javaHome` would be to trigger forking Java
4. a large amount of unnecessary classloading was happening due to #4744

### Solution

1. Set the default `zinc.analysis.cache.limit` value to `Int.MaxValue`, which will allow users who want to cap the size to set it lower, but otherwise not lead to performance cliffs when a target has more dependencies than the current limit.
2. Implicitly disable log4j JMX usage by setting the `log4j2.disable.jmx` property if it is not already set.
3. Remove the `forkJava` setting, to prepare to use the `javaHome` setting explicitly in #4729
4. Enable usage of `ClassLoaderCache` 

### Result

Fixes #4744, and removes a few more blockers for #4729.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants