SI-9632 don't keep JarFile open in ZipArchive (with -Dscala.classpath.closeZip=true) #5592
Conversation
|
The JVM internally caches the underlying (native) datastructures for simultaneously open Under this patch, we wouldn't expect to benefit from this in the single-compiler case, however, we would benefit if multiple compilers happen to be reading some entry from the same JAR at the same time. Interestingly, the JVM sometimes returns the cached internal datastructure in the sequential case, but I can't reconcile this with my understanding of the implementation. Any JVM / JNI / C gurus reading that could explain that puzzle, based on Without this caching, the JNI implementation of We could also add a |
| val name = zipEntry.getName() | ||
| val time = zipEntry.getTime() | ||
| val size = zipEntry.getSize().toInt | ||
| class FileEntry() extends Entry(name) { |
There was a problem hiding this comment.
I'd suggest moving this class out to an enclosing scope where it can see openZipFile but not zipEntry and zipFile. (Basically, to make the capture explicit as constructor arguments.)
There was a problem hiding this comment.
Yup I can do that. In fact I do it that way with the other approach.
There's an argument to interning the names here... it is a considerable proportion of the heap in my builds.
There was a problem hiding this comment.
You might profit from -XX:+UseStringDeduplication (JEP-192), which lazily deduplicates the backing Array[Char] of long lived Strings as part of GC.
There was a problem hiding this comment.
I use that, it don't seem to do very much. I've been a bit disappointed by it.
There was a problem hiding this comment.
Also, g1 seems down my work compile significantly. Default flags win.
| new FilterInputStream(delegate) { | ||
| override def close(): Unit = { | ||
| delegate.close() | ||
| zipFile.close() |
There was a problem hiding this comment.
zipFile.close will automatically close delegate, but it is harmless to do it explicitly.
There was a problem hiding this comment.
what do you think about using a bytearray to just read and close here?
There was a problem hiding this comment.
If someone happened to be using this code to read zip entries and stream the contents into a file, or some other processing, that would be counter-productive. For the known user of this code in the compiler, it would be slightly counter productive, as the caller would just copy that Array immediately. We could of course change the caller (AbstractFileReader.<init>'s call to AbstractFile.toByteArray), but in the interests of keeping this change small I'd avoid starting down that path.
|
is the build failure relevant? |
|
That seems to be caused by this change. |
diff --git a/src/reflect/scala/reflect/io/ZipArchive.scala b/src/reflect/scala/reflect/io/ZipArchive.scala
index f58780580a..bd21e149eb 100644
--- a/src/reflect/scala/reflect/io/ZipArchive.scala
+++ b/src/reflect/scala/reflect/io/ZipArchive.scala
@@ -140,14 +140,14 @@ final class FileZipArchive(file: JFile) extends ZipArchive(file) {
if (zipEntry.isDirectory) dir
else {
// allows zipEntry to be garbage collected
- val name = zipEntry.getName()
- val time = zipEntry.getTime()
- val size = zipEntry.getSize().toInt
- class FileEntry() extends Entry(name) {
- override def lastModified: Long = time // could be stale
+ val entryName = zipEntry.getName()
+ val entryTime = zipEntry.getTime()
+ val entrySize = zipEntry.getSize().toInt
+ class FileEntry() extends Entry(entryName) {
+ override def lastModified: Long = entryTime // could be stale
override def input: InputStream = {
val zipFile = openZipFile()
- val entry = zipFile.getEntry(name)
+ val entry = zipFile.getEntry(entryName)
val delegate = zipFile.getInputStream(entry)
new FilterInputStream(delegate) {
override def close(): Unit = {
@@ -156,7 +156,7 @@ final class FileZipArchive(file: JFile) extends ZipArchive(file) {
}
}
}
- override def sizeOption: Option[Int] = Some(size) // could be stale
+ override def sizeOption: Option[Int] = Some(entrySize) // could be stale
}
val f = new FileEntry()
dir.entries(f.name) = f
|
|
Oh I bet this is about visibility of variables in early initialisers or something. I've seen subtle changes result in nulls before. The proposed class refactor should fix that. There are loads of puzzler topics in this part of the code... |
|
it is a bit less subtle: the inside the anonymous class, inherited methods
have a higher binding precedence than the vals in the enclosing scope.
|
|
ah, that could be the cause all along. Another reason might be because I removed the getArchive impl... thinking nobody used it (the super says it only exists to oblige the sbt api) |
|
ok, updated, let's see if this fixes it. |
I was interpreting the output as meaning that native ZIP handling code was getting a cache hit for a brief period after the java |
|
Oh, I didn't realise the test was run again. I'll investigate shortly. My suspicion is that something is using the method that i removed, which leaks jarfiles. |
|
The test failure isn't your fault this time. Our binary compatbility tool, MiMa, briefly had a bug that reported a false positive for changes like yours. Rebasing on #5608 (which updates the version of MiMa used in the 2.12.x branch), and shuffling the code around (https://github.com/scala/scala/compare/2.12.x...retronym:review/5592?expand=1), should give a green build. In my branch, I've guarded the new code with a system property. I think this it makes sense to get some real world reports about how this works before turning it on by default. |
|
Is there a way to get a community build timing? I think that's a good metric. I don't think my work project is representative of the "real world" in this sense. |
|
Most of the time is spent resolving dependencies and executing tests. We don't have a way to extract performance metrics of just the compilation. |
|
Here's the impact on a microbenchmark: This suggests a 30% slowdown as penalty for reading the JAR metadata for each entry, rather than just once. I just threw the benchmark together, so the usual caveats apply. While the time spent reading JARs might be a small fraction of the compiler performance, I'm still uncomfortable taking this hit without giving the new code some time to be battle tested. |
|
I suspect this will disproportionately impact smaller files where more time is spent in I/O, but on bigger projects with lots of type inference it's unlikely to have an impact. Feature flagging it is a wise choice. |
|
updated, I implemented the property switch... but I have no idea how to put this behind a |
|
mmm, I don't like the name EagerEntry... that implies it loads the contents straight away. I'll change that to |
|
I'm struggling to see where the failure is in the jenkins log, could somebody please help? |
|
|
ah, so failing forwards compatibility. I guess if I make these private it will be ok? |
|
they're already private enough as far as Scala is concerned (see lightbend-labs/mima#34 and lightbend-labs/mima#53), so it would be fine to whitelist them |
|
is there a doc on how to do that? |
|
Should be add these to |
|
Awesome, thanks! |
|
done |
|
green light, green light! 😺 @SethTisue if this is merged, could you please run a version of the community build with and without this flag enabled? There is an environment variable that java accepts for global flags... I'm trying my best to remember it. |
|
darn, conflicts I didn't notice. I'll try to clean this up. |
|
rebased |
|
/rebuild |
|
rebased |
It's merged as of 8 days ago, FYI. |
|
|
|
Thanks Seth! Does this have the flag enabled? |
I just sort of robotically triggered a run for you because you asked, but now that I read through this PR in more detail, I see that you actually just want the runs to see how long they take, not just to see whether they pass. I'm afraid that won't work. first, the variability in the run timings is too large, because the Jenkins workers might or might not be doing something else. if your thing gives a 10x slowdown, we'll find out, but we won't get finer-grained information than that. you could run the community build locally and reduce the variability that way, but it still wouldn't give good information for a more fundamental reason, which is that the community build runtime isn't actually dominated by running the compiler. it's a chunk of the overall time, sure, but resolving dependencies is also a huge chunk, and compiling build definitions (with Scala 2.10, not the compiler being tested) is a chunk, and running tests is a huge chunk. we don't know the relative sizes of these chunks and we don't currently have a way of measuring that. basically, the community build isn't a compiler benchmark and I don't think there's any way to use it as one. it so happens that Jason and Lukas have been developing an actual compiler-benchmarking system lately; I'll leave it to Jason to comment whether that might be applicable here. |
|
It's also good to see if this triggers any regressions with the flag enabled. Is there anything I need to do? |
|
@fommil I started a test run with A simple way to test it would be to push a commit to this PR which enables it by default, which triggers tests. That could also be used for a community build. That commit can be removed afterwards. |
|
For running the compiler benchmarks, we're still setting things up. We don't have a way yet to run it on PRs, but that's coming soon. |
|
@lrytz @SethTisue time for a bit of magic 😉 ... if you use |
|
Now that is interesting magic. More info. |
|
Anything I need to do? |
I’m only speculating, but perhaps the comments here got so extensive, with so many tangents (e.g. into community build stuff), that for Jason to review all that and swap the current state back in is daunting, especially given how many other PRs there are in our queue. perhaps you could try to add a comment here summarizing the current status as you see it? |
|
I think this is good to go and I'm not aware of any more review comments to address. I'd like to see a community build with the envvar I referenced above, to test correctness of this new option, if that's possible. |
|
Yep, let's get this one in. The new feature is not on by default, and the refactorings around the existing code path appear safe. |
|
Thanks! I can delete our ensime hack now :-) |
| if (zipEntry.isDirectory) dir | ||
| else { | ||
| class FileEntry() extends Entry(zipEntry.getName) { | ||
| override def getArchive = zipFile |
There was a problem hiding this comment.
Both new impls don't override this, leaving it default to null. Is that ok? The comment says:
// have to keep this name for compat with sbt's compiler-interface
def getArchive: ZipFile = nullThere was a problem hiding this comment.
Yup, intentional. It isn't used and is leaky even if it was.
There was a problem hiding this comment.
Yeah it's not been used since 2011 / sbt 0.11.0:
sbt/sbt@ff95799#diff-97684450c7452ac0dbc1eace52decd97L154
👍
This is a proposed fix for https://issues.scala-lang.org/browse/SI-9632
It is absolutely essential that this does not introduce a performance regression, so I'd like to hear how we can be sure about that.
Timings of the community build are one way to check, but that's expensive. So I wrote https://github.com/fommil/sbt-scala-monkey as a way to try this out for any sbt project (this file should work for 2.11). I've not noticed any performance slowdown on my projects (my work project is very big).
While working on this I noticed a few things that are worth mentioning here:
Array[Byte]and returning it, to be sure that the jar file is closed? I could use BasicIO toreadFully, but I never trusted that it is performant enough.PS: I go one further than this patch when working on a very large windows codebase at work, where instead of opening the JarFile to read entries, I use the URL classloader and I apply https://github.com/fommil/class-monkey, which will cache entries and (other than timestamp checks) results in zero IO accesses for the second compile. I'm happy to raise a PR with that approach, to get an idea of the impact it has (when not using class-monkey), see https://gist.github.com/fommil/5709abc1678b346b6d927a13cd30b844#file-ziparchive-scala-L140-L223