From be489e05caeb4db85d7aa5e3a36f0bf340f74cc7 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Sun, 28 Jul 2019 15:08:22 -0700 Subject: [PATCH 1/3] Clear expired loaders Sometimes turbo mode didn't work correctly for projects where resources were modified. This was because it was possible for the resource classloader to inadvertently evict the dependency classloader from the classloader cache because they had the same file stamps. There were two fixes: 1) remove expired entries from the cache based on the (Parent, Classpath) pair rather than just classpath 2) do not close the classloaders during cache eviction. They may still be in use when we evict them so we need to wait until they are explicitly closed elsewhere or until the go out of scope and are collected by the CleanupThread I tested this change with a spark project in which I kept modifying the resources. Prior to this change, I could get into a state where if I modified the resources, the dependency layer would get evicted every time so the benefits of turbo mode were not realized. --- .../sbt/internal/classpath/WrappedLoader.java | 10 ++++++ .../internal/classpath/ClassLoaderCache.scala | 34 ++++++++++++------- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/main-command/src/main/java/sbt/internal/classpath/WrappedLoader.java b/main-command/src/main/java/sbt/internal/classpath/WrappedLoader.java index 8bf7b7edc..932187f65 100644 --- a/main-command/src/main/java/sbt/internal/classpath/WrappedLoader.java +++ b/main-command/src/main/java/sbt/internal/classpath/WrappedLoader.java @@ -9,8 +9,10 @@ package sbt.internal.classpath; import java.net.URL; import java.net.URLClassLoader; +import java.util.concurrent.atomic.AtomicBoolean; public class WrappedLoader extends URLClassLoader { + private final AtomicBoolean invalidated = new AtomicBoolean(false); static { ClassLoader.registerAsParallelCapable(); } @@ -19,6 +21,14 @@ public class WrappedLoader extends URLClassLoader { super(new URL[] {}, parent); } + void invalidate() { + invalidated.set(true); + } + + boolean invalidated() { + return invalidated.get(); + } + @Override public URL[] getURLs() { final ClassLoader parent = getParent(); diff --git a/main-command/src/main/scala/sbt/internal/classpath/ClassLoaderCache.scala b/main-command/src/main/scala/sbt/internal/classpath/ClassLoaderCache.scala index c0683d040..eaf3ff1de 100644 --- a/main-command/src/main/scala/sbt/internal/classpath/ClassLoaderCache.scala +++ b/main-command/src/main/scala/sbt/internal/classpath/ClassLoaderCache.scala @@ -71,18 +71,26 @@ private[sbt] class ClassLoaderCache( new java.util.concurrent.ConcurrentHashMap[Key, Reference[ClassLoader]]() private[this] val referenceQueue = new ReferenceQueue[ClassLoader] - private[this] def closeExpiredLoaders(): Unit = { - val toClose = lock.synchronized(delegate.asScala.groupBy(_._1.files.toSet).flatMap { + private[this] def clearExpiredLoaders(): Unit = lock.synchronized { + val clear = (k: Key, ref: Reference[ClassLoader]) => { + ref.get() match { + case w: WrappedLoader => w.invalidate() + case _ => + } + delegate.remove(k) + () + } + def isInvalidated(classLoader: ClassLoader): Boolean = classLoader match { + case w: WrappedLoader => w.invalidated() + case _ => false + } + delegate.asScala.groupBy { case (k, _) => k.parent -> k.files.toSet }.foreach { case (_, pairs) if pairs.size > 1 => - val max = pairs.maxBy(_._1.maxStamp)._1 - pairs.filterNot(_._1 == max).flatMap { - case (k, v) => - delegate.remove(k) - Option(v.get) - } - case _ => Nil - }) - toClose.foreach(close) + val max = pairs.map(_._1.maxStamp).max + pairs.foreach { case (k, v) => if (k.maxStamp != max) clear(k, v) } + case _ => + } + delegate.forEach((k, v) => if (isInvalidated(k.parent)) clear(k, v)) } private[this] class CleanupThread(private[this] val id: Int) extends Thread(s"classloader-cache-cleanup-$id") { @@ -97,7 +105,7 @@ private[sbt] class ClassLoaderCache( delegate.remove(key) case _ => } - closeExpiredLoaders() + clearExpiredLoaders() false } catch { case _: InterruptedException => true @@ -178,7 +186,7 @@ private[sbt] class ClassLoaderCache( val ref = mkReference(key, f()) val loader = ref.get delegate.put(key, ref) - closeExpiredLoaders() + clearExpiredLoaders() loader } lock.synchronized { From 621789eeb25ab2056db700631b3b017d1aab74ed Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Mon, 29 Jul 2019 11:52:42 -0700 Subject: [PATCH 2/3] Remove resource layer for AllDependencyJars strategy Changed resources were causing the dependency layer to be invalidated on resource changes in turbo mode because the resource layer was in between the scala library layer. This commit reworks the layers for the AllDependencyJars strategy so that the top layer is able to load _all_ of the resources during a test run. The resource layer was added to address the problem that dependencies may need to be able to load resources from the project classpath but wouldn't be able to do so if the dependencies were in a separate layer from the rest of the classpath. The resource layer was a classloader that could load any resource on the full classpath but couldn't load any classes. When I added the resource layer, I was thinking that when resources changed, the resource class loader needed to be invalidated. Resources, however, are different from classes in that the same ClassLoader can find the same resources in a different place because getResource and getResourceAsStream just return locations but do not actually do any loading. Taking advantage of this, I add a proxy classloader for finding resource locations to ReverseLookupClassLoader. We can reset the classpath of the resource loader in ReverseLookupClassLoaderHolder.checkout. This allows us to see the new versions of the resources without invalidating the dependency layer. --- .../java/sbt/internal/ResourceLoader.java | 23 ---------- .../scala/sbt/internal/ClassLoaders.scala | 45 ++++--------------- .../sbt/internal/LayeredClassLoaders.scala | 21 +++++++-- 3 files changed, 25 insertions(+), 64 deletions(-) delete mode 100644 main/src/main/java/sbt/internal/ResourceLoader.java diff --git a/main/src/main/java/sbt/internal/ResourceLoader.java b/main/src/main/java/sbt/internal/ResourceLoader.java deleted file mode 100644 index 9d7993c64..000000000 --- a/main/src/main/java/sbt/internal/ResourceLoader.java +++ /dev/null @@ -1,23 +0,0 @@ -/* - * sbt - * Copyright 2011 - 2018, Lightbend, Inc. - * Copyright 2008 - 2010, Mark Harrah - * Licensed under Apache License 2.0 (see LICENSE) - */ - -package sbt.internal; - -import java.io.File; -import scala.collection.immutable.Map; -import scala.collection.Seq; - -final class ResourceLoader extends ResourceLoaderImpl { - ResourceLoader( - final Seq classpath, final ClassLoader parent, final Map resources) { - super(classpath, parent, resources); - } - - static { - ClassLoader.registerAsParallelCapable(); - } -} diff --git a/main/src/main/scala/sbt/internal/ClassLoaders.scala b/main/src/main/scala/sbt/internal/ClassLoaders.scala index 1650efc9e..866c2964b 100644 --- a/main/src/main/scala/sbt/internal/ClassLoaders.scala +++ b/main/src/main/scala/sbt/internal/ClassLoaders.scala @@ -35,12 +35,10 @@ private[sbt] object ClassLoaders { if (si.isManagedVersion) rawCP else si.libraryJars.map(j => j -> IO.getModifiedTimeOrZero(j)).toSeq ++ rawCP val exclude = dependencyJars(exportedProducts).value.toSet ++ si.libraryJars - val resourceCP = modifiedTimes((outputFileStamps in resources).value) buildLayers( strategy = classLoaderLayeringStrategy.value, si = si, fullCP = fullCP, - resourceCP = resourceCP, allDependencies = dependencyJars(dependencyClasspath).value.filterNot(exclude), cache = extendedClassLoaderCache.value, resources = ClasspathUtilities.createClasspathResources(fullCP.map(_._1), si), @@ -55,7 +53,6 @@ private[sbt] object ClassLoaders { val s = streams.value val opts = forkOptions.value val options = javaOptions.value - val resourceCP = modifiedTimes((outputFileStamps in resources).value) if (fork.value) { s.log.debug(s"javaOptions: $options") Def.task(new ForkRun(opts)) @@ -85,7 +82,6 @@ private[sbt] object ClassLoaders { strategy = classLoaderLayeringStrategy.value: @sbtUnchecked, si = instance, fullCP = classpath.map(f => f -> IO.getModifiedTimeOrZero(f)), - resourceCP = resourceCP, allDependencies = transformedDependencies, cache = extendedClassLoaderCache.value: @sbtUnchecked, resources = ClasspathUtilities.createClasspathResources(classpath, instance), @@ -118,7 +114,6 @@ private[sbt] object ClassLoaders { strategy: ClassLoaderLayeringStrategy, si: ScalaInstance, fullCP: Seq[(File, Long)], - resourceCP: Seq[(File, Long)], allDependencies: Seq[File], cache: ClassLoaderCache, resources: Map[String, String], @@ -156,34 +151,28 @@ private[sbt] object ClassLoaders { } .getOrElse(scalaLibraryLayer) - // layer 2 (resources) - val resourceLayer = - if (layerDependencies) - getResourceLayer(cpFiles, resourceCP, scalaReflectLayer, cache, resources) - else scalaReflectLayer - - // layer 3 (optional if in the test config and the runtime layer is not shared) - val dependencyLayer = + // layer 2 (optional if in the test config and the runtime layer is not shared) + val dependencyLayer: ClassLoader = if (layerDependencies && allDependencies.nonEmpty) { cache( allDependencies.toList.map(f => f -> IO.getModifiedTimeOrZero(f)), - resourceLayer, - () => new ReverseLookupClassLoaderHolder(allDependencies, resourceLayer) + scalaReflectLayer, + () => new ReverseLookupClassLoaderHolder(allDependencies, scalaReflectLayer) ) - } else resourceLayer + } else scalaReflectLayer val scalaJarNames = (si.libraryJars ++ scalaReflectJar).map(_.getName).toSet - // layer 4 + // layer 3 val filteredSet = if (layerDependencies) allDependencies.toSet ++ si.libraryJars ++ scalaReflectJar else Set(si.libraryJars ++ scalaReflectJar: _*) val dynamicClasspath = cpFiles.filterNot(f => filteredSet(f) || scalaJarNames(f.getName)) dependencyLayer match { case dl: ReverseLookupClassLoaderHolder => - dl.checkout(dynamicClasspath, tmp) + dl.checkout(cpFiles, tmp) case cl => cl.getParent match { - case dl: ReverseLookupClassLoaderHolder => dl.checkout(dynamicClasspath, tmp) + case dl: ReverseLookupClassLoaderHolder => dl.checkout(cpFiles, tmp) case _ => new LayeredClassLoader(dynamicClasspath, cl, tmp) } } @@ -194,24 +183,6 @@ private[sbt] object ClassLoaders { key: sbt.TaskKey[Seq[Attributed[File]]] ): Def.Initialize[Task[Seq[File]]] = Def.task(data(key.value).filter(_.getName.endsWith(".jar"))) - // Creates a one or two layered classloader for the provided classpaths depending on whether - // or not the classpath contains any snapshots. If it does, the snapshots are placed in a layer - // above the regular jar layer. This allows the snapshot layer to be invalidated without - // invalidating the regular jar layer. If the classpath is empty, it just returns the parent - // loader. - private def getResourceLayer( - classpath: Seq[File], - resources: Seq[(File, Long)], - parent: ClassLoader, - cache: ClassLoaderCache, - resourceMap: Map[String, String] - ): ClassLoader = { - if (resources.nonEmpty) { - val mkLoader = () => new ResourceLoader(classpath, parent, resourceMap) - cache(resources.toList, parent, mkLoader) - } else parent - } - // helper methods private def flatLoader(classpath: Seq[File], parent: ClassLoader): ClassLoader = new FlatLoader(classpath.map(_.toURI.toURL).toArray, parent) diff --git a/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala b/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala index 468615dcf..869c5e39c 100644 --- a/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala +++ b/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala @@ -8,7 +8,7 @@ package sbt.internal import java.io.File -import java.net.URLClassLoader +import java.net.{ URL, URLClassLoader } import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.atomic.{ AtomicBoolean, AtomicReference } @@ -69,7 +69,7 @@ private[internal] final class ReverseLookupClassLoaderHolder( * * @return a ClassLoader */ - def checkout(dependencyClasspath: Seq[File], tempDir: File): ClassLoader = { + def checkout(fullClasspath: Seq[File], tempDir: File): ClassLoader = { if (closed.get()) { val msg = "Tried to extract class loader from closed ReverseLookupClassLoaderHolder. " + "Try running the `clearCaches` command and re-trying." @@ -79,8 +79,8 @@ private[internal] final class ReverseLookupClassLoaderHolder( case null => new ReverseLookupClassLoader case c => c } - reverseLookupClassLoader.setTempDir(tempDir) - new BottomClassLoader(dependencyClasspath, reverseLookupClassLoader, tempDir) + reverseLookupClassLoader.setup(tempDir, fullClasspath) + new BottomClassLoader(fullClasspath, reverseLookupClassLoader, tempDir) } private def checkin(reverseLookupClassLoader: ReverseLookupClassLoader): Unit = { @@ -149,6 +149,19 @@ private[internal] final class ReverseLookupClassLoaderHolder( private[this] val classLoadingLock = new ClassLoadingLock def isDirty: Boolean = dirty.get() def setDescendant(classLoader: BottomClassLoader): Unit = directDescendant.set(classLoader) + private[this] val resourceLoader = new AtomicReference[ResourceLoader](null) + private class ResourceLoader(cp: Seq[File]) + extends URLClassLoader(cp.map(_.toURI.toURL).toArray, parent) { + def lookup(name: String): URL = findResource(name) + } + private[ReverseLookupClassLoaderHolder] def setup(tmpDir: File, cp: Seq[File]): Unit = { + setTempDir(tmpDir) + resourceLoader.set(new ResourceLoader(cp)) + } + override def findResource(name: String): URL = resourceLoader.get() match { + case null => null + case l => l.lookup(name) + } def loadClass(name: String, resolve: Boolean, reverseLookup: Boolean): Class[_] = { classLoadingLock.withLock(name) { try super.loadClass(name, resolve) From 6686e833b15d2e4cb62e1bb6cc8db6d09282a1d3 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Mon, 29 Jul 2019 12:18:45 -0700 Subject: [PATCH 3/3] Sort dependency jars I realized that it would be a good idea to sort the dependencyJars so that they appear in the same order that they do in the fullClasspath. --- main/src/main/scala/sbt/internal/ClassLoaders.scala | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/main/src/main/scala/sbt/internal/ClassLoaders.scala b/main/src/main/scala/sbt/internal/ClassLoaders.scala index 866c2964b..4c01831bb 100644 --- a/main/src/main/scala/sbt/internal/ClassLoaders.scala +++ b/main/src/main/scala/sbt/internal/ClassLoaders.scala @@ -39,7 +39,7 @@ private[sbt] object ClassLoaders { strategy = classLoaderLayeringStrategy.value, si = si, fullCP = fullCP, - allDependencies = dependencyJars(dependencyClasspath).value.filterNot(exclude), + allDependenciesSet = dependencyJars(dependencyClasspath).value.filterNot(exclude).toSet, cache = extendedClassLoaderCache.value, resources = ClasspathUtilities.createClasspathResources(fullCP.map(_._1), si), tmp = IO.createUniqueDirectory(taskTemporaryDirectory.value), @@ -82,7 +82,7 @@ private[sbt] object ClassLoaders { strategy = classLoaderLayeringStrategy.value: @sbtUnchecked, si = instance, fullCP = classpath.map(f => f -> IO.getModifiedTimeOrZero(f)), - allDependencies = transformedDependencies, + allDependenciesSet = transformedDependencies.toSet, cache = extendedClassLoaderCache.value: @sbtUnchecked, resources = ClasspathUtilities.createClasspathResources(classpath, instance), tmp = taskTemporaryDirectory.value: @sbtUnchecked, @@ -114,7 +114,7 @@ private[sbt] object ClassLoaders { strategy: ClassLoaderLayeringStrategy, si: ScalaInstance, fullCP: Seq[(File, Long)], - allDependencies: Seq[File], + allDependenciesSet: Set[File], cache: ClassLoaderCache, resources: Map[String, String], tmp: File, @@ -137,6 +137,7 @@ private[sbt] object ClassLoaders { } val cpFiles = fullCP.map(_._1) + val allDependencies = cpFiles.filter(allDependenciesSet) val scalaReflectJar = allDependencies.collectFirst { case f if f.getName == "scala-reflect.jar" => si.allJars.find(_.getName == "scala-reflect.jar")