From 8c4b8ff01de8d861b99da2c8941a34976d1ed9d3 Mon Sep 17 00:00:00 2001 From: jjudd Date: Tue, 29 Oct 2024 14:54:38 -0600 Subject: [PATCH] Fix AnnexScalaInstance jar cache misses due to ordering and path comparison problems The jar cache in AnnexScalaInstance is a ConcurrentHashMap that uses a string representation of the compiler classpath (a list of jars) as its key. Problem is that list of jars which is used to create the key is not sorted, so you would sometimes get cache misses for the same classpath because the list had a different ordering. At a large enough scale, this would cause the JVM to run out of CodeHeap and crash. Presumably this was because we'd create up to n! AnnexScalaInstances for every AnnexScalaInstance and then classload for every single one of those instances. This change prevents those cache misses and thus prevents the worker from crashing. This should also fix cache misses with path mapping by comparing the short path, so we don't include the bazel arch/config specific parts of the path. --- .../workers/common/AnnexScalaInstance.scala | 23 +++++++++++------ .../rules_scala/workers/common/FileUtil.scala | 25 ++++++++++++++++++- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/src/main/scala/higherkindness/rules_scala/workers/common/AnnexScalaInstance.scala b/src/main/scala/higherkindness/rules_scala/workers/common/AnnexScalaInstance.scala index 4d3af8e4..60cbd3cf 100644 --- a/src/main/scala/higherkindness/rules_scala/workers/common/AnnexScalaInstance.scala +++ b/src/main/scala/higherkindness/rules_scala/workers/common/AnnexScalaInstance.scala @@ -7,7 +7,7 @@ import java.net.URLClassLoader import java.nio.file.{Files, Path, Paths} import java.util.Properties import java.util.concurrent.ConcurrentHashMap -import scala.collection.immutable.Map +import scala.collection.immutable.TreeMap object AnnexScalaInstance { // See the comment on getAnnexScalaInstance as to why this is necessary @@ -48,7 +48,7 @@ object AnnexScalaInstance { * from a prior compilation in combination with a new ScalaInstance for the current compilation. Or there's some issue * with Scala's classpath caching. * - * If all three caches are enabled (this cache + Zinc claspath + Scala compiler), then the non-determinism seems go + * If all three caches are enabled (this cache + Zinc claspath + Scala compiler), then the non-determinism seems to go * away. * * If this cache is not used, but the Zinc classpath + Scala compiler caches are used, then non-determinsm shows up in @@ -67,11 +67,20 @@ object AnnexScalaInstance { * any different from leaving Zinc's cache enabled. */ private def getAnnexScalaInstance(allJars: Array[File], workDir: Path): AnnexScalaInstance = { - // We need to remove the sandbox prefix from the paths in order to compare them. - val mapBuilder = Map.newBuilder[Path, Path] + // We want to compare short paths to avoid the Bazel sandbox prefix and arch/config specific parts of the path + // We use a tree map because we want the keys sorted for comparison purposes + val mapBuilder = TreeMap.newBuilder[Path, Path] + val absoluteWorkDir = workDir.toAbsolutePath().normalize() allJars.foreach { jar => - val comparableJarPath = jar.toPath().toAbsolutePath().normalize() - mapBuilder.addOne(jar.toPath -> workDir.toAbsolutePath().normalize().relativize(comparableJarPath)) + val absoluteJarPath = jar.toPath().toAbsolutePath().normalize() + // Remove the sandbox prefix as well as the arch/config specific parts of the path. + mapBuilder.addOne( + jar.toPath -> + FileUtil.bazelShortPath( + absoluteWorkDir.relativize(absoluteJarPath), + replaceExternal = false, + ), + ) } val workRequestJarToWorkerJar = mapBuilder.result() @@ -84,7 +93,7 @@ object AnnexScalaInstance { // but that would require hashing the all the classpath jars on every compilation request. I // imagine that would cause a performance hit. // - // I also imagine it is extremeley rare to be mutating the contents of compiler classpath jars + // I also imagine it is extremely rare to be mutating the contents of compiler classpath jars // while keeping the names the same, e.g., generating new scala library jar for scala 2.13.14. // As a result I'm leaving this string based for now. val key = workRequestJarToWorkerJar.values.mkString(":") diff --git a/src/main/scala/higherkindness/rules_scala/workers/common/FileUtil.scala b/src/main/scala/higherkindness/rules_scala/workers/common/FileUtil.scala index 2e6f762a..0f2ba349 100644 --- a/src/main/scala/higherkindness/rules_scala/workers/common/FileUtil.scala +++ b/src/main/scala/higherkindness/rules_scala/workers/common/FileUtil.scala @@ -4,7 +4,7 @@ package workers.common import scala.annotation.tailrec import java.io.{File, IOException} import java.nio.channels.FileChannel -import java.nio.file.{FileAlreadyExistsException, FileVisitResult, Files, Path, SimpleFileVisitor, StandardCopyOption, StandardOpenOption} +import java.nio.file.{FileAlreadyExistsException, FileVisitResult, Files, Path, Paths, SimpleFileVisitor, StandardCopyOption, StandardOpenOption} import java.nio.file.attribute.BasicFileAttributes import java.util.zip.{ZipEntry, ZipInputStream, ZipOutputStream} @@ -59,6 +59,29 @@ object FileUtil { path.getFileName().toString().stripPrefix("header_").stripPrefix("processed_") } + /** + * Given a Bazel path to the bazel output directory, return a path excluding the bazel configuration specific parts of + * the path. This is analogous to Bazel's File.short_path function. Fair warning: this function is super good enough, + * but is likely not perfect. + */ + def bazelShortPath(path: Path, replaceExternal: Boolean = true): Path = { + val nameCount = path.getNameCount() + val pathString = path.toAbsolutePath().normalize().toString() + + val shortPath = if (path.startsWith("bazel-out") && nameCount >= 4) { + path.subpath(3, nameCount) + } else { + path + } + + // Handle difference between Bazel's external directory being referred to as .. in the short_path + if (replaceExternal && shortPath.startsWith("external")) { + Paths.get(shortPath.toString().replaceFirst("external", "..")) + } else { + shortPath + } + } + def copy(source: Path, target: Path) = Files.walkFileTree(source, new CopyFileVisitor(source, target)) def delete(path: Path) = Files.walkFileTree(path, new DeleteFileVisitor)