On Wed, 11 Sep 2024 08:34:09 GMT, Eirik Bjørsnøs <[email protected]> wrote:
>> 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 investigation, 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 Just a guess but perhaps this is down to a cache effect where the `ZipEntry` allocation has a chance to evict the cen data array from some cache. Batching all the reads from CEN together could counter-act some such effects and better streamline memory accesses. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20939#discussion_r1753891457
