Skip to content

Commit

Permalink
Zinc 1.0.0-RC3 memory and output improvements (#4807)
Browse files Browse the repository at this point in the history
### 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.
  • Loading branch information
Stu Hood authored Aug 11, 2017
1 parent 9f415be commit a965d45
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 10 deletions.
9 changes: 8 additions & 1 deletion src/scala/org/pantsbuild/zinc/analysis/AnalysisMap.scala
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,14 @@ class AnalysisMap private[AnalysisMap] (
}

object AnalysisMap {
private val analysisCacheLimit = Util.intProperty("zinc.analysis.cache.limit", 100)
// Because the analysis cache uses Weak references, bounding its size is generally
// counterproductive.
private val analysisCacheLimit =
Util.intProperty(
"zinc.analysis.cache.limit",
Int.MaxValue
)

/**
* Static cache for compile analyses. Values must be Options because in get() we don't yet
* know if, on a cache miss, the underlying file will yield a valid Analysis.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ case class CompilerCacheKey(
compilerBridgeSrc: File,
compilerInterface: File,
javaHome: Option[File],
forkJava: Boolean,
cacheDir: File)

object CompilerCacheKey {
Expand All @@ -40,7 +39,6 @@ object CompilerCacheKey {
compilerBridgeSrc,
compilerInterface,
settings.javaHome,
settings.forkJava,
settings.zincCacheDir
)
}
Expand All @@ -55,7 +53,6 @@ object CompilerCacheKey {
compilerBridgeSrc: File,
compilerInterface: File,
javaHomeDir: Option[File],
forkJava: Boolean,
cacheDir: File
): CompilerCacheKey = {
val normalise: File => File = { _.getAbsoluteFile }
Expand All @@ -65,6 +62,6 @@ object CompilerCacheKey {
val compilerBridgeJar = normalise(compilerBridgeSrc)
val compilerInterfaceJar = normalise(compilerInterface)
val javaHome = javaHomeDir map normalise
CompilerCacheKey(compilerJar, libraryJar, extraJars, compilerBridgeJar, compilerInterfaceJar, javaHome, forkJava, cacheDir)
CompilerCacheKey(compilerJar, libraryJar, extraJars, compilerBridgeJar, compilerInterfaceJar, javaHome, cacheDir)
}
}
11 changes: 8 additions & 3 deletions src/scala/org/pantsbuild/zinc/compiler/CompilerUtils.scala
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import sbt.internal.inc.{
javac,
ZincUtil
}
import sbt.internal.inc.classpath.ClassLoaderCache
import sbt.io.Path
import sbt.io.syntax._
import sbt.util.Logger
Expand Down Expand Up @@ -59,6 +60,12 @@ object CompilerUtils {
CompilerCache.createCacheFor(maxCompilers)
}

/**
* Cache of classloaders: see https://github.com/pantsbuild/pants/issues/4744
*/
private val classLoaderCache: Option[ClassLoaderCache] =
Some(new ClassLoaderCache(new URLClassLoader(Array())))

/**
* Get or create a zinc compiler based on compiler setup.
*/
Expand Down Expand Up @@ -86,9 +93,7 @@ object CompilerUtils {
ZincCompilerUtil.constantBridgeProvider(instance, interfaceJar),
ClasspathOptionsUtil.auto,
_ => (),
// TODO: Should likely use the classloader cache here:
// see https://github.com/pantsbuild/pants/issues/4744
None
classLoaderCache
)

/**
Expand Down
4 changes: 4 additions & 0 deletions src/scala/org/pantsbuild/zinc/compiler/Main.scala
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ object Main {
def printVersion(): Unit = println("%s (%s) %s" format (Command, Description, versionString))

def mkLogger(settings: Settings) = {
// If someone has not explicitly enabled log4j2 JMX, disable it.
if (!Util.isSetProperty("log4j2.disable.jmx")) {
Util.setProperty("log4j2.disable.jmx", "true")
}
val cl =
ConsoleLogger(
out = ConsoleOut.systemOut,
Expand Down
2 changes: 0 additions & 2 deletions src/scala/org/pantsbuild/zinc/compiler/Settings.scala
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ case class Settings(
scala: ScalaLocation = ScalaLocation(),
scalacOptions: Seq[String] = Seq.empty,
javaHome: Option[File] = None,
forkJava: Boolean = false,
_zincCacheDir: Option[File] = None,
javaOnly: Boolean = false,
javacOptions: Seq[String] = Seq.empty,
Expand Down Expand Up @@ -255,7 +254,6 @@ object Settings extends OptionSet[Settings] {

header("Java options:"),
file( "-java-home", "directory", "Select javac home directory (and fork)", (s: Settings, f: File) => s.copy(javaHome = Some(f))),
boolean( "-fork-java", "Run java compiler in separate process", (s: Settings) => s.copy(forkJava = true)),
string( "-compile-order", "order", "Compile order for Scala and Java sources", (s: Settings, o: String) => s.copy(compileOrder = compileOrder(o))),
boolean( "-java-only", "Don't add scala library to classpath", (s: Settings) => s.copy(javaOnly = true)),
prefix( "-C", "<javac-option>", "Pass option to javac", (s: Settings, o: String) => s.copy(javacOptions = s.javacOptions :+ o)),
Expand Down
8 changes: 8 additions & 0 deletions src/scala/org/pantsbuild/zinc/util/Util.scala
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,14 @@ object Util {
// Properties
//

def setProperty(name: String, value: String): Unit = {
System.setProperty(name, value)
}

def isSetProperty(name: String): Boolean = {
System.getProperty(name) ne null
}

/**
* Create int from system property.
*/
Expand Down

0 comments on commit a965d45

Please sign in to comment.