8352642: Set zipinfo-time=false when constructing zipfs FileSystem in com.sun.tools.javac.file.JavacFileManager$ArchiveContainer for better performance

[retronym]

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

• JDK-8352642: Set zipinfo-time=false when constructing zipfs FileSystem in com.sun.tools.javac.file.JavacFileManager$ArchiveContainer for better performance (Enhancement - P4)

Reviewers

• Chen Liang (@liach - Reviewer)
• Lance Andersen (@LanceAndersen - Reviewer)
• Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24176/head:pull/24176
$ git checkout pull/24176

Update a local copy of the PR:
$ git checkout pull/24176
$ git pull https://git.openjdk.org/jdk.git pull/24176/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24176

View PR using the GUI difftool:
$ git pr show -t 24176

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24176.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jzaugg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@retronym This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8352642: Set zipinfo-time=false when constructing zipfs FileSystem in com.sun.tools.javac.file.JavacFileManager$ArchiveContainer for better performance

Reviewed-by: liach, lancea, jpai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 19 new commits pushed to the master branch:

• ba658a7: 8349522: AArch64: Add backend implementation for new unsigned and saturating vector operations
• 5625b43: 8350429: runtime/NMT/CheckForProperDetailStackTrace.java should only run for debug JVM
• 2c60fc5: 8352176: Automate setting up environment for mac signing tests
• 6e6a39d: 8347321: [ubsan] CGGlyphImages.m:553:30: runtime error: nan is outside the range of representable values of type 'unsigned long'
• b84b292: 8352615: [Test] RISC-V: TestVectorizationMultiInvar.java fails on riscv64 without rvv support
• a54445f: 8350609: Cleanup unknown unwind opcode (0xB) for windows
• c87e1be: 8349582: APX NDD code generation for OpenJDK
• 7d1fe0e: 8339543: [vectorapi] laneHelper and withLaneHelper should be ForceInline
• ddc4318: 8352628: Refine Grapheme test
• d8c2f59: 8352064: AIX: now also able to build static-jdk image with a statically linked launcher
• ... and 9 more: https://git.openjdk.org/jdk/compare/df9210e6578acd53384ee1ac06601510c9a52696...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@liach, @LanceAndersen, @jaikiran) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@retronym The following label will be automatically applied to this pull request:

• compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[retronym]

The else branch, which uses FileSystems.newFileSystem(archivePath, (ClassLoader)null) rather than jarFsProvider.newFileSystem, is unchanged in the PR.

AFAICT that would only be taken when a client using JavacFileManager directly, omitting the fm.handleOption(Option.MULTIRELEASE, .. ) call, or in a custom scenario with non-JAR archivess.

[lahodaj]

I wonder if there's some suspicion of a particular problem in the other branch, or other reason to not add the flag there as well. I would expect the default should be to add it to both branches, unless there's a fairly strong reason not to.

There are some more new jar FileSystems created in Locations, but those only typically read one file, so probably not that big deal w.r.t. this flag.

[retronym]

There are some more new jar FileSystems created in Locations, but those only typically read one file, so probably not that big deal w.r.t. this flag.

Agreed. I initially changed those as well but when I realized they only read a single file I reverted.

I wonder if there's some suspicion of a particular problem in the other branch,

The then branch, which I've modified, was introduced in e652402 (JDK-8149757: Implement Multi-Release JAR aware JavacFileManager for javac). It became necessary to ensure exact usage of ZipFileSystemProvider to be able to pass in the env map with the multiRelease config.

I reviewed the discussion of that change. There was a mention of the introduction of this branch, but it didn't go as far to suggest removal of the FileSystems.newFileSystem call, it only justified the addition of the jarFsProvider.newFileSystem call.

https://mail.openjdk.org/pipermail/compiler-dev/2016-April/thread.html

> -why is JavacFileManager.getJarFSProvider() needed? Shouldn't
> FileSystems.newFileSystem(realPath, env);
> be enough? (Calling an SPI directly from an API client seems 
> suspicious to me.)

If I recall correctly, the NIO2 API design forced this one. The method 
you are referring to does not exist. There is only one that takes a URI, 
and that has very different semantics.  So we had to go with the method 
on the provider, hence the use of getJarFSProvider.

The tests introduced in e652402 don't appear to exercise the else branch.

It appears there are two ways to get there:

1. Direct, programatic use of the a file manager in which the initialization sequence does not populate multiReleaseValue
2. Use of javac with a .zip as a classpath entry (I had to check, but this does work.) Side question -- what is the expected behaviour with a classpath entry with a non-lowercase extension, .JAR? It is allowed by javac but would not respect the multi-release setting.

[lahodaj]

I think that's all true, but I am not sure if it quite explains why not do the same for the same for the else section. There is FileSystems.newFileSystem(Path path, Map<String,?> env, ClassLoader loader) since JDK 13, so there shouldn't be a big problem to call that one instead of FileSystems.newFileSystem(Path path, ClassLoader loader) (the latter delegates to the former, so there shouldn't be any real change in semantics).

I guess one concern could be that, in theory, this method works for other FS providers/types as well. But, presumably, those would ignore env keys they don't know, and would ignore it.

[mlbridge]

Webrevs

• 00: Full (eee2f613)

[liach]

Looks good. Indeed JavacTask sets Option.MULTIRELEASE explicitly. Might need another zip area contributor to comment on such usages, as there is no usage of this property in the jdk codebase besides one specific test.

[liach]

/label core-libs
/reviewers 2 reviewer

[openjdk]

@liach
The core-libs label was successfully added.

[openjdk]

@liach
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

[AlanBateman]

I think this is okay. I was chatting with Jai and Lance about the undocumented property and I think we've agreed there is merit to the semantics (or close to it) for what is there now, but give it a more suitable name. It could then be documented in the jdk.zipfs's module description.

[retronym]

but give it a more suitable name

I'm assuming this would come in a followup change for the ZipFS maintainers, rather this in this PR.

[jaikiran]

but give it a more suitable name

I'm assuming this would come in a followup change for the ZipFS maintainers, rather this in this PR.

Yes, that's correct.

[liach]

Can any zip area developer leave an approval?

[jaikiran]

I'm running some tests with this change. I'll approve it once that completes.

[lahodaj]

Good catch, thanks! Ordinarily, I would say a test is required, but I can't think up a viable test to verify the effect of this change in a test, so we probably need to do it without a test.

[liach]

The tier 1-3 tests submitted by jaikiran all pass. This should be good to go.

[retronym]

Ordinarily, I would say a test is required, but I can't think up a viable test to verify the effect of this change in a test, so we probably need to do it without a test.

I tested this change externally with JFRUnit.

If preferred, I could create a jtreg test along similar lines. It would use the JFR API directly to capture jdk.FileRead events. The test would construct a JAR that previously triggered one read per entry, and assert that after this change, only a single read is needed to initialize a compiler whose classpath includes that JAR.

[jaikiran]

Hello Jason, I was crafting some JAR files to compose a large classpath and running some compilation tests on a Windows system to see if there's a observable difference with this change. But I see that you have done the necessary testing externally with much more precise checks and those do show the expected improvements with reduced native calls to read. I won't pursue my Windows testing, since what you have is good both from the code change as well as testing point of view.

I could create a jtreg test along similar lines. ... The test would construct a JAR that previously triggered one read per entry, and assert that after this change, only a single read is needed to initialize a compiler whose classpath includes that JAR.

I don't think that can be guaranteed/asserted and could lead to an intermittent failing or brittle test.

[jaikiran]

Looks good to me. Before integrating, you might want to wait for Jan to have a chance to review and respond to the discussion you have been having about the "else" block and the possibility of introducing a test.