JAR reading: more accurate and widespread caching, less read contention by retronym · Pull Request #9557 · scala/scala

retronym · 2021-03-30T04:42:25Z

Multithreaded users of the compiler can experience lock contention in
classpath reading. This occurs because a) scalac by default reuses a
ZipFile instance for a given JAR across concurrent compilers to reduce the
memory footprint, and b) because j.u.ZipFile contains does not support
concurrent reads.

Instead of having a 1-1 relationship between ZipArchive and j.u.ZipFile,
this PR uses an internal pool of ZipFile instances, which can be expanded
as needed to service concurrent reads a given JAR file.

While investigating this, I noticed that our caching layer was not
considering the value of the -release compiler option as part of the
cache key. This is now fixed. I also included Java 9+ classpath reposotories
(JrtClassPath and CtSymClassPath in the caching scheme.)

Profiling showed my that the attempt to reduce allocations when converting
the classpath entry names into symbol names was counter productive as it
bypassed lazy val name in ClassFileEntryImpl which was forced anyway on
other code paths.

dwijnand · 2021-03-30T06:58:46Z

% scalac t6591_3.scala
% <in process execution of run/t6591_3.scala> > t6591_3-run.log
% diff /home/jenkins/workspace/scala-2.12.x-validate-main/test/files/run/t6591_3.check /home/jenkins/workspace/scala-2.12.x-validate-main/test/files/run/t6591_3-run.log
@@ -1 +1,13 @@
 Block(List(ValDef(Modifiers(), TermName("v"), Select(This(TypeName("A")), TypeName("I")), Apply(Select(New(Select(This(TypeName("A")), TypeName("I"))), termNames.CONSTRUCTOR), List()))), Ident(TermName("v")))
+Uncaught exception on thread Thread[Timer-0,5,main]: java.nio.file.NoSuchFileException: t6502-run.obj/test1.jar
+java.nio.file.NoSuchFileException: t6502-run.obj/test1.jar
+	at sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)
+	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
+	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)
+	at sun.nio.fs.UnixPath.toRealPath(UnixPath.java:837)
+	at com.sun.nio.zipfs.ZipFileSystemProvider.removeFileSystem(ZipFileSystemProvider.java:322)
+	at com.sun.nio.zipfs.ZipFileSystem.close(ZipFileSystem.java:313)
+	at scala.tools.nsc.classpath.ZipFsClassPath.close(DirectoryClassPath.scala:220)
+	at scala.tools.nsc.classpath.FileBasedCache$$anon$1$$anon$2.run(ZipAndJarFileLookupFactory.scala:249)
+	at java.util.TimerThread.mainLoop(Timer.java:555)
+	at java.util.TimerThread.run(Timer.java:505)

smarter · 2021-03-31T00:52:34Z

What are the downsides of this option that prevent using it by default?

retronym · 2021-03-31T01:01:29Z

Nothing major other than the risk of new code. I'd like to battle test it to flush out further bugs and refine the performance.

I'm not sure if it is shipped with non-OpenJDK based distros, so we might need the old implementation as a fallback.

retronym · 2021-04-01T01:34:34Z

A clarification: java.nio.zipfs.ZipFileSystem does not actually support concurerent reads of data, but it does support concurrent reads of metadata.

retronym · 2021-04-01T04:33:55Z

My earlier analysis was wrong, I do see contention on reads in this implementation, which is visible in my multi-threaded compilation benchmark. The improvement that I thought I saw might have been due to #9549 instead.

retronym · 2021-04-13T06:21:15Z

Update:

I've tried to make zipfs lock free on reads by tweaking the way it does File I/O. 8265448: Avoid lock contention in reads from zipfs when possible retronym/jdk#2. I'm floating this idea with the JDK team. https://twitter.com/retronym/status/1381823131875086336
I've pushed another commit here that uses a pool of plain-old java.util.ZipFiles to avoid the contention but avoid needing to open a ZipFile instance for each class we read.

This prevents concurrent compilers with different values for this compiler option from seeing the incorrect API.

Classpath elements based on a) jrt:// file system (representing platform libraries of the current the Java 9+ instance) and b) ct.sym (the JEP 247 repository of the of previous JDK versions) are an immutable part of the JDK. The ClassPath entries we create are safe to share across concurrent or subsequent compilers in the same way we cache entries for regular JARs.

Classpath caching shares a single instance of ZipArchive across multiple threads. This can cause read contention as j.u.ZipFile internally serializes reads. Instead, maintain a pool of ZipFile instances to avoid sharing them across threads.

The current optimized version tries to avoid temporary strings. But it doesn't achieve this for classes based by jrt:// (or any `NioPath`, as the call to `AbstractFile.fileName` internally constructs a string each time. This commit uses `.name` (which is a `lazy val`).

The compiler has a per-classloader cache that backs the classpath lookups of individual instances of `Global`. Elements in the cache are used by `Global` instances that are concurrent (think parallel compilation of sub-projects) or are sequential within a small timeout. In scala#9557, this was extended to the classpath entry that backs the `scalac -release` compiler option ([JEP-247](https://openjdk.java.net/jeps/247) support for viewing the Java base library "as of" an older JDK version. This change was buggy -- it did not include the selected release in the cache key, which could lead to a compiler that specifies `-release X` seeing the results of another compiler using `-release Y`. This behaviour was tested by a JDK-9+ conditional test (`MultiReleaseJarTest`) which unfortunately is not part of our CI on the 2.12.x branch, so the regression went unnoticed. While in this area, I followed up on a TODO comment in the same test and discovered another bug in handling of multi-release JARs. Again, this bug could manifest when different values of `-release` were used in a build. It would manifest as an `IllegalArgumentException` in `ResuableDataReader` when it used the size of the non-versioned classfile when sizing buffers for the versioned classfile.

scala-jenkins added this to the 2.12.14 milestone Mar 30, 2021

retronym force-pushed the faster/zipfs branch from 1cf09ea to 5b89018 Compare March 30, 2021 05:51

retronym force-pushed the faster/zipfs branch 2 times, most recently from bc4baed to 80a942d Compare March 31, 2021 00:08

retronym force-pushed the faster/zipfs branch from 80a942d to 137911a Compare March 31, 2021 02:45

retronym force-pushed the faster/zipfs branch from 137911a to 45e5c24 Compare April 1, 2021 01:51

retronym marked this pull request as draft April 1, 2021 03:44

dwijnand mentioned this pull request Apr 6, 2021

Integrate splain – implicit resolution chains and type diffs in error messages #7785

Merged

SethTisue added the release-notes worth highlighting in next release notes label Apr 8, 2021

retronym force-pushed the faster/zipfs branch from c1e6f9e to 32ea22d Compare May 4, 2021 06:06

retronym added 4 commits May 4, 2021 16:07

Include --release version in cache key for classpath cache

493f985

This prevents concurrent compilers with different values for this compiler option from seeing the incorrect API.

Avoid contention in classpath access

31255c3

Classpath caching shares a single instance of ZipArchive across multiple threads. This can cause read contention as j.u.ZipFile internally serializes reads. Instead, maintain a pool of ZipFile instances to avoid sharing them across threads.

retronym force-pushed the faster/zipfs branch from 32ea22d to d0396cd Compare May 4, 2021 06:07

retronym changed the title ~~JAR reading: option to use zipfs, other fixes~~ JAR reading: more accurate and widespread caching, less read contention May 4, 2021

retronym requested a review from lrytz May 17, 2021 20:21

retronym marked this pull request as ready for review May 17, 2021 20:22

lrytz approved these changes May 18, 2021

View reviewed changes

lrytz merged commit 21a8a7a into scala:2.12.x May 18, 2021

retronym mentioned this pull request Jun 22, 2021

Include -release arg in cache key for ct.sym classpath element #9671

Merged

SethTisue mentioned this pull request Nov 2, 2021

Update zinc fork with the newest fixes scalacenter/bloop#1600

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JAR reading: more accurate and widespread caching, less read contention#9557

JAR reading: more accurate and widespread caching, less read contention#9557
lrytz merged 4 commits intoscala:2.12.xfrom
retronym:faster/zipfs

retronym commented Mar 30, 2021 •

edited

Loading

Uh oh!

dwijnand commented Mar 30, 2021

Uh oh!

smarter commented Mar 31, 2021 •

edited

Loading

Uh oh!

retronym commented Mar 31, 2021

Uh oh!

retronym commented Apr 1, 2021

Uh oh!

retronym commented Apr 1, 2021

Uh oh!

retronym commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

retronym commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwijnand commented Mar 30, 2021

Uh oh!

smarter commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

retronym commented Mar 31, 2021

Uh oh!

retronym commented Apr 1, 2021

Uh oh!

retronym commented Apr 1, 2021

Uh oh!

retronym commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

retronym commented Mar 30, 2021 •

edited

Loading

smarter commented Mar 31, 2021 •

edited

Loading