cache JrtFileSystem in ClasspathJrt.jrtFileSystem #2815

[jukzi]

to avoid ephemeral memory allocations

#2815

[stephan-herrmann]

Since the change is not small, I'd very much appreciate a description of your strategy. What is being improved and how?

[jukzi]

Since the change is not small, I'd very much appreciate a description of your strategy. What is being improved and how?

Don't know what it not clear: instead of calling JRTUtil.method(File, ... ) i outlined and used JRTUtil.method(JrtFileSystem, ... ). The JrtFileSystem used is eager created in advance in the constructor ClasspathJrt(String zipFilename) and stored in ClasspathJrt.jrtFileSystem instead of just the Filename "jrtFile. Then it can be reused in subsequent calls. For example in the heavy used ClasspathJrt.isPackage().
I also added modifiers "private final" as much as possible for all members, which required to inline ClasspathJrtWithReleaseOption.initialize()

[stephan-herrmann]

Since the change is not small, I'd very much appreciate a description of your strategy. What is being improved and how?

Don't know what it not clear:

initially, when looking at the PR "nothing" is clear, since we can't read the intentions inside your head :)

instead of calling JRTUtil.method(File, ... ) i outlined and used JRTUtil.method(JrtFileSystem, ... ).

I don't see "instead", but "additionally / optionally", meaning that only one client of JRTUtil is affected, others will still use the old strategy, right?

The JrtFileSystem used is eager created in advance in the constructor ClasspathJrt(String zipFilename) and stored in ClasspathJrt.jrtFileSystem instead of just the Filename "jrtFile. Then it can be reused in subsequent calls. For example in the heavy used ClasspathJrt.isPackage().

ok

I also added modifiers "private final" as much as possible for all members, which required to inline ClasspathJrtWithReleaseOption.initialize()

thanks, that explains the other "big" change :)

FYI, I'm asking these questions mainly so we better know what's going on when later we have to revisit this code in another bug report.

[jukzi]

Such questions are very welcome as they help me to challenge my code. Now about the benefit of this change: I took a measurement of building the platform workspace within the IDE. Sampling did yield this:

With this change the same usecase results in:

One can see ClasspathJrt.isPackage() is reduced by ~1 second and so does the the overall build time in org.eclipse.core.internal.events.BuildManager.basicBuildLoop(). So i am sure the time did not just move to the constructor :-)
Note that ClasspathJrt.findClass() is also reported to be reduced, but it includes a call to isPackage():

The changes assumes the JRT on the disk does does not change during the runtime. Due to the eager init there might be behavioral changes if the JRT is not found on disk. That should not happen anyway.

I did not improve the Batch compiler callers as i am not interested in that. Probably same optimization could be done for org.eclipse.jdt.internal.compiler.batch.ClasspathJrt.file

If there are no concerns i plan to submit next M1.

[jukzi]

Regarding the batch compiler it seems straight forward to add a batch.ClasspathJrt.jrtFileSystem but i don't understand how/if it should be used as argument for
org.eclipse.jdt.internal.compiler.tool.EclipseFileManager.getJrtFileSystem(File)
and why the batch compiler never calls getJrtSystem(, release) with any non-null release as ClasspathJrtWithReleaseOption does.

[stephan-herrmann]

why the batch compiler never calls getJrtSystem(, release) with any non-null release

have you seen FileSystem.getOlderSystemRelease()?

Also, looking at initialize() methods being called in constructors of ..batch.ClasspathJrt and subclasses I don't see any problem with memory / initialization. Do you?

[HannesWell]

Thank you Jörg for this one.
I think this is going into the right direction.

[HannesWell]

Is it really necessary to keep these static methods that just call a similarly named method on the first argument here and below?
Some handle the case where the JRTFileSystem is null, but I wonder if that would be better handled by the caller?
And some don't even do null-checks and just call the corresponding method on JRTFileSystem.

[jukzi]

I suggest you answer that question yourself by looking into the call hierarchy.

[HannesWell]

If I do that I see that one just has to change the visibility of the called method to public and then the same question arises, plus should that method not be public (together with the corresponding other methods)?

But in general, since I patiently answer your question and address your remarks on my PRs, I was hoping that you would return the favor and answer my questions/remarks on your PRs.

[jukzi]

There are 4 callers. i don't see a benefit to add the null check to all of them.

[jukzi]

why the batch compiler never calls getJrtSystem(, release) with any non-null release

have you seen FileSystem.getOlderSystemRelease()?

they store release in ClasspathJep247.compliance, but never call getJrtSystem. i don't get why batch compiler have two different "ClasspathJrt" implementations.

Also, looking at initialize() methods being called in constructors of ..batch.ClasspathJrt and subclasses I don't see any problem with memory / initialization. Do you?

I don't see a problem in the source code but i have never used or even measured the batch compiler in any way. I will add a commit for changes that can be similar done in batch compiler - without knowing how big the benefit will be there.