On Tue, 10 Sep 2024 19:39:34 GMT, Claes Redestad <redes...@openjdk.org> wrote:
>> Eirik Bjørsnøs has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add whitespace per review feedback > > src/java.base/share/classes/java/util/zip/ZipFile.java line 681: > >> 679: >> 680: e.flag = CENFLG(cen, pos); >> 681: e.method = CENHOW(cen, pos); > > Not reading `nlen` when it's not needed is a good change, and moving `clen` > and `elen` down to be grouped with the others is fine, but some of the other > shuffling around here doesn't seem motivated? The reordering of field reads was motivated by some previous experiences where ordering the reads sequentially with their appearance in the CEN a had small, but positive performance gain. After closer invetigation, it turns out that the internal ordering of field reads only has small benefits, maybe in the noise. However, reading the length fields _before the allocation of the `ZipEntry`_ has a significant negative impact. In fact, it seems to explain most of the performance gains in this PR. It seems that having `ZipEntry` allocation interspersed within the CEN field reads incurs a significant cost. I can't explain why, but perhaps @cl4es can? (To reproduce, simply move the length reads to the beginning of the method) I have kept the reordering of `nlen`, `elen`, `clen` reads, but reverted some other reorderings to make the PR cleaner. This PR: Benchmark (size) Mode Cnt Score Error Units ZipFileGetEntry.getEntryHit 512 avgt 15 52.057 ? 2.021 ns/op ZipFileGetEntry.getEntryHit 1024 avgt 15 54.753 ? 1.093 ns/op Reads length fields before `ZipEntry` allocation: Benchmark (size) Mode Cnt Score Error Units ZipFileGetEntry.getEntryHit 512 avgt 15 65.199 ? 0.823 ns/op ZipFileGetEntry.getEntryHit 1024 avgt 15 69.407 ? 0.807 ns/op ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20939#discussion_r1753604647
