8321620: Optimize JImage decompressors

[Glavo]

I generated runtime images using jlink --compress 2 --add-modules java.se,jdk.unsupported,jdk.management and then ran the following JMH benchmark:

@Warmup(iterations = 10, time = 2)
@Measurement(iterations = 5, time = 3)
@Fork(value = 1, jvmArgsAppend = {"-XX:+UseG1GC", "-Xms8g", "-Xmx8g", "--add-exports=java.base/jdk.internal.jimage=ALL-UNNAMED"})
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Benchmark)
public class Decompress {

    private static final ImageReader READER = ImageReaderFactory.getImageReader();
    private static final ImageLocation LOC = READER.findLocation("java.base", "java/lang/String.class");

    @Benchmark
    public ByteBuffer test() {
        return READER.getResourceBuffer(LOC);
    }

}

This is the result:

Zip

Benchmark                           Mode  Cnt       Score       Error   Units        Score       Error   Units
Decompress.test                     avgt    5  194237.534 ±  1026.180   ns/op   152855.728 ± 16058.780   ns/op   (-21.30%)
Decompress.test:gc.alloc.rate       avgt    5    1197.700 ±     6.306  MB/sec      464.278 ±    47.465  MB/sec
Decompress.test:gc.alloc.rate.norm  avgt    5  243953.338 ±     5.810    B/op    74376.291 ±     2.175    B/op   (-69.51%)
Decompress.test:gc.count            avgt    5       2.000              counts        1.000              counts
Decompress.test:gc.time             avgt    5       4.000                  ms        3.000                  ms

The results show that memory allocation is reduced by 70% while decompression speed is significantly improved.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8321620: Optimize JImage decompressors (Enhancement - P4)

Reviewers

• Mandy Chung (@mlchung - Reviewer)
• Claes Redestad (@cl4es - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17405

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back Glavo! 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]

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

• core-libs

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.

[mlbridge]

Webrevs

• 00: Full (741c18d0)

[mlchung]

Looks good to me. Should wait for @cl4es review as he looked at the previous version.

[openjdk]

@Glavo 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:

8321620: Optimize JImage decompressors

Reviewed-by: mchung, redestad

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 9 new commits pushed to the master branch:

• bdee968: 4760025: sRGB conversions to and from CIE XYZ incorrect
• 71d9a83: 8323243: JNI invocation of an abstract instance method corrupts the stack
• d83ea92: 8301466: [AIX] Revisit CommittedVirtualMemoryTest
• 5cf7947: 8323562: SaslInputStream.read() may return wrong value
• dc7d3b1: 8321489: Update LCMS to 2.16
• 84cf4cb: 8318563: GetClassFields should not use random access to field
• 9e9c05f: 8322979: Add informative discussion to Modifier
• c54bca6: 8323617: Add missing null checks to GetMousePositionWithPopup.java test
• 95a9168: 8323627: Shenandoah: Refactor init logger

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 (@mlchung, @cl4es) 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).

[Glavo]

I ran tier1~tier2 tests and there were no new failures.

[cl4es]

Looks good to me!

Pre-existing issue here is that it seems we don't handle compressed resources that can't fit in a single byte[], yet we take the overhead of storing uncompressed/compressed size as longs (4+4 byte overhead per entry). I guess we should either add code to handle huge entries or optimize the compressed header to store int sizes.

[Glavo]

/integrate

[openjdk]

@Glavo
Your change (at version 741c18d) is now ready to be sponsored by a Committer.

[Glavo]

Pre-existing issue here is that it seems we don't handle compressed resources that can't fit in a single byte[], yet we take the overhead of storing uncompressed/compressed size as longs (4+4 byte overhead per entry). I guess we should either add code to handle huge entries or optimize the compressed header to store int sizes.

This limitation is so deep-rooted that it is difficult to resolve it for the time being. But I don't think this is causing any problems for the time being. There is no need to put an overly large resource in a jimage file, it will be better placed elsewhere.

But even so, I think it's appropriate to store the size as 64 bits, which will make it easier to lift this restriction in the future. If you want the size of the metadata, I think it is more appropriate to compress the metadata. I tried using zstd to compress the metadata in JApp and got good results. We might be able to do this in jimage as well.

[cl4es]

I'd very much welcome support for zstd, both for resource content and metadata. A larger JEP-sized project, that.

I have run a quick experiment with adding a more compact header format (16-bit sizes for the four fields) and on a jlink --compress 2 --add-modules java.se,jdk.unsupported,jdk.management it reduces the jimage size by about 0.9% (33279195 vs 32983412 bytes). Throughput might take a small hit. I'm not sure I have time right now to get tested and PR'd but I'll post the draft after sponsoring this.

[cl4es]

/sponsor

[openjdk]

Going to push as commit a03eb6d.
Since your change was applied there have been 27 commits pushed to the master branch:

• f5b757c: 8323159: Consider adding some text re. memory zeroing in Arena::allocate
• 1f4474f: 8323726: Serial: Remove unused definitions in Generation
• dd0694b: 8323671: DevKit build gcc libraries contain full paths to source location
• bf813be: 8322279: Generational ZGC: Use ZFragmentationLimit and ZYoungCompactionLimit as percentage instead of multiples
• c84c0ab: 8323637: Capture hotspot replay files in GHA
• f368a0c: 8320328: Restore interrupted flag in ImageIcon.loadImage
• a45b5b4: 8323722: Serial: Remove unused no_gc_in_progress
• 7e0a4ed: 8323101: C2: assert(n->in(0) == nullptr) failed: divisions with zero check should already have bailed out earlier in split-if
• 34f85ee: 8323584: AArch64: Unnecessary ResourceMark in NativeCall::set_destination_mt_safe
• 62fd26f: 8323700: Add fontconfig requirement to building.md for Alpine Linux
• ... and 17 more: https://git.openjdk.org/jdk/compare/999e556be4302de4b6911e6d62ee5ca556a76469...master

Your commit was automatically rebased without conflicts.

[openjdk]

@cl4es @Glavo Pushed as commit a03eb6d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[openjdk]

@cl4es The command sponsor can only be used in open pull requests.

[cl4es]

For reference here's my draft idea on a backwards-compatible compressed compressed header (heh): #17433

No significant difference on micros but a modest saving on the archive size.

[Glavo]

I'd very much welcome support for zstd, both for resource content and metadata. A larger JEP-sized project, that.

I have run a quick experiment with adding a more compact header format (16-bit sizes for the four fields) and on a jlink --compress 2 --add-modules java.se,jdk.unsupported,jdk.management it reduces the jimage size by about 0.9% (33279195 vs 32983412 bytes). Throughput might take a small hit. I'm not sure I have time right now to get tested and PR'd but I'll post the draft after sponsoring this.

From my experience on the JApp project, if compressed metadata is implemented, the size of these fields may be irrelevant.

In the early development stage of JApp, I used 32-bit integers to store metadata such as resource size. But after using zstd to compress the metadata, I was surprised to find that storing them as 64-bit integers can actually reduce the file size for some test cases. The compression algorithm does the work for me, so I no longer need to care about these details.

[Glavo]

In the early development stage of JApp, I used 32-bit integers to store metadata such as resource size. But after using zstd to compress the metadata, I was surprised to find that storing them as 64-bit integers can actually reduce the file size for some test cases.

Sorry, I re-looked at the test data and found that I was wrong. Switching 32-bit fields to 64-bit still slightly increased the file size, although I still feel this is inconsequential.

[cl4es]

It'd be very interesting to see zstd compression applied in this context to see how it'd stack up w.r.t archive size, speed of decompression and so on.

In this specific case we'd probably have to keep the 4-byte magic marker intact (it might be possible to rid us of this), then compress the remainder (25 bytes) and store the size (a byte might be enough). For reference with regular zip on representative data I can squeeze headers down then from 29 to 22-24 bytes. In #17433 I'm down to 12 bytes (including marker) with little to no overhead. I'm not going to pursue getting #17433 integrated (for now), but it might be a nice little piece of a larger puzzle.