Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v10]
On Mon, 23 Oct 2023 13:04:20 GMT, Sean Coffey wrote: >> Fix up java.util.zip.ZipFile$Source hashCode() impl so that duplicate Source >> objects aren't created for the same zip file. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Incorporate test review comments from Jai I think the latest changes are fine Sean - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16115#pullrequestreview-1692683963
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v10]
On Mon, 23 Oct 2023 13:04:20 GMT, Sean Coffey wrote: >> Fix up java.util.zip.ZipFile$Source hashCode() impl so that duplicate Source >> objects aren't created for the same zip file. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Incorporate test review comments from Jai Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16115#pullrequestreview-1692563327
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v10]
On Mon, 23 Oct 2023 13:04:20 GMT, Sean Coffey wrote: >> Fix up java.util.zip.ZipFile$Source hashCode() impl so that duplicate Source >> objects aren't created for the same zip file. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Incorporate test review comments from Jai Thank you for the updates, Sean. The test `ZipSourceCache` looks good to me in its current form. I have no other review comments except for the jmh benchmark. I'll approve this PR in its current form. As for the jmh benchmark, the existing benchmark method has some informative inline comment explaining what it does. Perhaps we should have something similar for this new benchmark method explaining why it is there? It currently says "// Ensure that we only create ZipFile.Source.Key entry per unique zip file" which doesn't seem accurate considering that this is a benchmark method and doesn't have any verifications of Key counts. I'll read up a bit more about jmh tomorrow because I'm unsure how multiple `@Benchmark` methods within a single class are run and what all those annotations mean when it comes to state management and such. But you don't have to wait for my approval of that part. If anyone else approves this in the meantime, please go ahead with your integration. - PR Comment: https://git.openjdk.org/jdk/pull/16115#issuecomment-1775197122
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v10]
On Mon, 23 Oct 2023 13:04:20 GMT, Sean Coffey wrote: >> Fix up java.util.zip.ZipFile$Source hashCode() impl so that duplicate Source >> objects aren't created for the same zip file. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Incorporate test review comments from Jai meant to add - note that `createZipFile` already prints an error to standard err in the scenario where the zip file can't be opened for modification. - PR Comment: https://git.openjdk.org/jdk/pull/16115#issuecomment-1775156218
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v10]
On Mon, 23 Oct 2023 13:04:20 GMT, Sean Coffey wrote: >> Fix up java.util.zip.ZipFile$Source hashCode() impl so that duplicate Source >> objects aren't created for the same zip file. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Incorporate test review comments from Jai Thanks for the review Jai. I've edited the test to use` ZipFile z = new ZipFile(relativeFile)` in both places. Regards the JMH benchmark, note that there is a significant performance gain for some environments with this patch. 2X improvement for filesystems that support fileKey() and deal with opening of a zip file twice (or more) but with different variants of path name. The benchmark update was useful for highlighting this. The existing ` src = files.get(key);` ZipFile code call is key to this improvement. Now that hashcode() is corrected, a new pairing is avoided for the same zip file. p.s. I've run test-repeat on mach5 for this new test a few times already. It was test repeat 20 or 30 IIRC. I'll bump it to 50 for last test round before integrating. - PR Comment: https://git.openjdk.org/jdk/pull/16115#issuecomment-1775148314
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v10]
> Fix up java.util.zip.ZipFile$Source hashCode() impl so that duplicate Source > objects aren't created for the same zip file. Sean Coffey has updated the pull request incrementally with one additional commit since the last revision: Incorporate test review comments from Jai - Changes: - all: https://git.openjdk.org/jdk/pull/16115/files - new: https://git.openjdk.org/jdk/pull/16115/files/3635e542..ff6c7d70 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16115=09 - incr: https://webrevs.openjdk.org/?repo=jdk=16115=08-09 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16115.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16115/head:pull/16115 PR: https://git.openjdk.org/jdk/pull/16115
