From 5dc671c7f814c68ef89ff538d09be32fd63198ab Mon Sep 17 00:00:00 2001 From: Mark Harrah Date: Wed, 19 Jun 2013 19:20:37 -0400 Subject: [PATCH] Synchronize ClassLoaderCache and the Scala provider cache. Construction of Scala providers was already properly synchronized jvm and machine-wide. The cache on top of construction was not and neither was the newer ClassLoaderCache. This could cause the same Scala version to be loaded in multiple class loaders, taking up more permgen space and possibly decreasing performance due to less effective jit. The issue is very rare in practice for 0.13 because of the low probability of contention on ClassLoaderCache. This is because the work for a cache miss is mainly the construction of a URLClassLoader. In 0.12, however, the work potentially involved network access and class loading (not just class loader construction), thus greatly increasing the probability of contention and thus duplicate work (i.e. class loader construction). When there is contention, multiple class loaders are constructed and then preserved by the scalaInstance task in each project throughout the first task execution. Only when multiple scalaInstance tasks execute simultaneously and only during the first execution does this occur. (Technically, it could still happen later, but it doesn't in practice.) This means that the number of duplicate class loaders should quickly saturate instead of growing linearly with the number of projects. It also means that the impact depends on the exact tree structure of projects. A linear chain of dependencies will be unaffected, but a build with independent leaves may be limited by the number of cores. The number of cores affects the number of threads typically used by the task engine, which limits the number of concurrently executing scalaInstance tasks. In summary, this might affect the first, cold compilation of a multi-module project with independent leaves on a multi-core machine with Scala version different from the version used for sbt. It might increase the maximum permgen requirements as well as slow the jit compilation by up to one task execution. Subsequent compilations should be unaffected and the permgen utilization return to be as expected. --- launch/src/main/scala/xsbt/boot/Cache.scala | 2 +- project/Sbt.scala | 2 +- .../project/scala-loader/project/Build.scala | 20 ++++++++++++ sbt/src/sbt-test/project/scala-loader/test | 7 +++++ .../sbt/classpath/ClassLoaderCache.scala | 6 +++- .../scala/sbt/classpath/ConcurrentCache.scala | 31 +++++++++++++++++++ 6 files changed, 65 insertions(+), 3 deletions(-) create mode 100644 sbt/src/sbt-test/project/scala-loader/project/Build.scala create mode 100644 sbt/src/sbt-test/project/scala-loader/test create mode 100644 util/classpath/src/test/scala/sbt/classpath/ConcurrentCache.scala diff --git a/launch/src/main/scala/xsbt/boot/Cache.scala b/launch/src/main/scala/xsbt/boot/Cache.scala index 488e5388e..669ea3819 100644 --- a/launch/src/main/scala/xsbt/boot/Cache.scala +++ b/launch/src/main/scala/xsbt/boot/Cache.scala @@ -9,7 +9,7 @@ import java.util.HashMap final class Cache[K,X,V](create: (K,X) => V) { private[this] val delegate = new HashMap[K,Reference[V]] - def apply(k: K, x: X): V = getFromReference(k, x, delegate.get(k)) + def apply(k: K, x: X): V = synchronized { getFromReference(k, x, delegate.get(k)) } private[this] def getFromReference(k: K, x: X, existingRef: Reference[V]) = if(existingRef eq null) newEntry(k, x) else get(k, x, existingRef.get) private[this] def get(k: K, x: X, existing: V) = if(existing == null) newEntry(k, x) else existing private[this] def newEntry(k: K, x: X): V = diff --git a/project/Sbt.scala b/project/Sbt.scala index 7921f7ffa..02407d802 100644 --- a/project/Sbt.scala +++ b/project/Sbt.scala @@ -60,7 +60,7 @@ object Sbt extends Build // Path, IO (formerly FileUtilities), NameFilter and other I/O utility classes lazy val ioSub = testedBaseProject(utilPath / "io", "IO") dependsOn(controlSub) settings(ioSettings : _ *) // Utilities related to reflection, managing Scala versions, and custom class loaders - lazy val classpathSub = baseProject(utilPath / "classpath", "Classpath") dependsOn(launchInterfaceSub, interfaceSub, ioSub) settings(scalaCompiler) + lazy val classpathSub = testedBaseProject(utilPath / "classpath", "Classpath") dependsOn(launchInterfaceSub, interfaceSub, ioSub) settings(scalaCompiler) // Command line-related utilities. lazy val completeSub = testedBaseProject(utilPath / "complete", "Completion") dependsOn(collectionSub, controlSub, ioSub) settings(jline : _*) // logging diff --git a/sbt/src/sbt-test/project/scala-loader/project/Build.scala b/sbt/src/sbt-test/project/scala-loader/project/Build.scala new file mode 100644 index 000000000..0a14c061a --- /dev/null +++ b/sbt/src/sbt-test/project/scala-loader/project/Build.scala @@ -0,0 +1,20 @@ +import sbt._ +import Keys._ + +object Build extends Build { + + lazy val checkLoader = TaskKey[Unit]("check-loaders") + + def checkTask = subs.map(sub => scalaInstance in LocalProject(sub.id)).join.map { sis => + assert(sis.sliding(2).forall{ case Seq(x,y) => x.loader == y.loader }, "Not all ScalaInstances had the same class loader.") + } + + override def projects = root +: subs + lazy val root = Project("root", file(".")).settings( checkLoader <<= checkTask, concurrentRestrictions := Nil ) + + lazy val subs = ( for(i <- 1 to 20) yield newProject(i) ).toSeq + + def newProject(i: Int): Project = Project("x" + i.toString, file(i.toString)).settings( + scalaVersion := "2.9.2" // this should be a Scala version different from the one sbt uses + ) +} diff --git a/sbt/src/sbt-test/project/scala-loader/test b/sbt/src/sbt-test/project/scala-loader/test new file mode 100644 index 000000000..1549c7f19 --- /dev/null +++ b/sbt/src/sbt-test/project/scala-loader/test @@ -0,0 +1,7 @@ +# verify that the class loader for Scala used for each project is the same +# this test failed in 0.12.4-RC2 and earlier. Even though 0.13.0-Beta2 +# technically had a problem as well, it is harder to make 0.13 exhibit +# problems in practice due to the different approach that substantially +# reduces the probability of simultaneous access. The better test for 0.13+ +# is the unit test on ClassLoaderCache +> check-loaders diff --git a/util/classpath/src/main/scala/sbt/classpath/ClassLoaderCache.scala b/util/classpath/src/main/scala/sbt/classpath/ClassLoaderCache.scala index a5e640860..2086c0ab0 100644 --- a/util/classpath/src/main/scala/sbt/classpath/ClassLoaderCache.scala +++ b/util/classpath/src/main/scala/sbt/classpath/ClassLoaderCache.scala @@ -8,7 +8,11 @@ import java.util.HashMap private[sbt] final class ClassLoaderCache(val commonParent: ClassLoader) { private[this] val delegate = new HashMap[List[File],Reference[CachedClassLoader]] - def apply(files: List[File]): ClassLoader = + + /** Returns a ClassLoader with `commonParent` as a parent and that will load classes from classpath `files`. + * The returned ClassLoader may be cached from a previous call if the last modified time of all `files` is unchanged. + * This method is thread-safe.*/ + def apply(files: List[File]): ClassLoader = synchronized { val tstamps = files.map(_.lastModified) getFromReference(files, tstamps, delegate.get(files)) diff --git a/util/classpath/src/test/scala/sbt/classpath/ConcurrentCache.scala b/util/classpath/src/test/scala/sbt/classpath/ConcurrentCache.scala new file mode 100644 index 000000000..359ef8c5a --- /dev/null +++ b/util/classpath/src/test/scala/sbt/classpath/ConcurrentCache.scala @@ -0,0 +1,31 @@ +package sbt +package classpath + +import org.scalacheck._ +import Prop._ +import java.io.File + +object ConcurrentCache extends Properties("ClassLoaderCache concurrent access") +{ + implicit lazy val concurrentArb: Arbitrary[Int] = Arbitrary( Gen.choose(1, 1000) ) + implicit lazy val filenameArb: Arbitrary[String] = Arbitrary( Gen.alphaStr ) + + property("Same class loader for same classpaths concurrently processed") = forAll { (names: List[String], concurrent: Int) => + withcp(names.distinct) { files => + val cache = new ClassLoaderCache(null) + val loaders = (1 to concurrent).par.map(_ => cache(files)).toList + sameClassLoader(loaders) + } + } + + private[this] def withcp[T](names: List[String])(f: List[File] => T): T = IO.withTemporaryDirectory { tmp => + val files = names.map{ name => + val file = new File(tmp, name) + IO.touch(file) + file + } + f(files) + } + private[this] def sameClassLoader(loaders: Seq[ClassLoader]): Boolean = loaders.size < 2 || + loaders.sliding(2).forall { case Seq(x,y) => x == y } +} \ No newline at end of file