8305995: Footprint regression from JDK-8224957

[catap]

I would like to backport this footprint regression from JDK-8224957 to JDK17.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8305995: Footprint regression from JDK-8224957 (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/1262/head:pull/1262
$ git checkout pull/1262

Update a local copy of the PR:
$ git checkout pull/1262
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/1262/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1262

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/1262.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back catap! 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.

[catap]

@vnkozlov, @TobiHartmann may I ask you to review this backport? Thanks.

[mlbridge]

Webrevs

• 01: Full - Incremental (f1c07898)
• 00: Full (d376554c)

[openjdk]

This backport pull request has now been updated with issue from the original commit.

[openjdk]

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

8305995: Footprint regression from JDK-8224957

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

• 12d08f0: 8308156: VerifyCACerts.java misses blank in error output
• e898048: 8307572: AArch64: Vector registers are clobbered by some macroassemblers
• f80be82: 8304976: Optimize DateTimeFormatterBuilder.ZoneTextPrinterParser.getTree()
• b6a5d20: 8269091: javax/sound/sampled/Clip/SetPositionHang.java failed with ArrayIndexOutOfBoundsException: Array index out of range: -4
• ef86ea2: 8283756: (zipfs) ZipFSOutputStreamTest.testOutputStream should only check inflated bytes
• ac9c696: 8301187: Memory leaks in OopMapCache
• c1e979f: 8300053: Shenandoah: Handle more GCCauses in ShenandoahControlThread::request_gc
• c9013a2: 8298138: Shenandoah: HdrSeq asserts "sub-bucket index (512) overflow for value ( 1.00)"
• 7765ba5: 8292698: Improve performance of DataInputStream
• 3d30c29: 8309107: Bump update version for OpenJDK: jdk-17.0.9
• ... and 100 more: https://git.openjdk.org/jdk17u-dev/compare/96adf073565e9a65912392510df8575e49695734...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.

➡️ 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).

[catap]

/integrate

[openjdk]

@catap
Your change (at version d376554) is now ready to be sponsored by a Committer.

[TobiHartmann]

You need approval for this backport, see https://openjdk.org/projects/jdk-updates/approval.html

[catap]

@TobiHartmann how can I file it without access to JBS? May I ask you to do it on my behalf?

[TobiHartmann]

Sure, but as I mentioned in openjdk/jdk#13453, we should give this some bake time, i.e. runs through testing, in mainline first. I'll follow up on this next week.

[catap]

@TobiHartmann thanks!

[phohensee]

Tagged the JBS issue.

[catap]

@phohensee thanks!

[catap]

@TobiHartmann, @phohensee any news?

[TobiHartmann]

The approval is still pending. This usually takes 1-2 weeks.

[tstuefe]

What tests were done?

Please enable Github actions in your repo, so that at least tier 1 GHAs run.

[catap]

@tstuefe actions are enabled. You may read discussion and motivation for that changes here: openjdk/jdk#13453

[tstuefe]

@tstuefe actions are enabled. You may read discussion and motivation for that changes here: openjdk/jdk#13453

@catap I don't see any checks: https://github.com/openjdk/jdk17u-dev/pull/1262/checks

We need tests in the backport release. Checks in backport releases are important since we are a lot closer to customer installations than we are in head, so backports are arguably more dangerous than patches in head.

Please see https://wiki.openjdk.org/display/JDKUpdates/How+to+contribute+or+backport+a+fix, especially section 3.

[openjdk]

@catap Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[catap]

/integrate

[catap]

@tstuefe force push triggers checks

[openjdk]

@catap
Your change (at version f1c0789) is now ready to be sponsored by a Committer.

[GoeLin]

Hi @catap, you may not integrate before you have jdk17u-fix-yes on the JBS issue.

Please

• tell what testing you did in addition to GHA. GHA is only a very basic sanity test.
• Reason why you want to have it in 17.0.8. We usually push to the same release as Oracle.
  (both this should go into the JBS issue, but I will look here as you don't have access to JBS)
  I'm removing the tag in the meantime. Add it again after supplying the information.

[catap]

Hi @GoeLin,

This regression was found while trying to migrate an application from JDK 1.8 to JDK 17, by running internal benchmarks and investigating abnormal memory usage for about 4 times more of one of them. This application is quite memory sensitive.

Outside of this application, I've discovered a lot of small wired things that are affected by this regression.

For example, I can't compile from IntelliJ IDEA fastutils when using JDK17 or JDK21, but I can when using JDK11. After my fix I can use both JDK17 and JDK21.

As part of the investigation I've been able to implement a benchmark that proves that the regression exists and allows to track that it has been fixed.

The idea of the benchmark is to build an RB tree containing a lot of primitive int and use JMH's GC profiler to measure the footprint.

When I run make test TEST="micro:RBTreeSearch" MICRO_OPTIONS="-prof gc" without backport of my fix I get an output:

Benchmark                                              Mode  Cnt     Score     Error   Units
RBTreeSearch.search                                   thrpt   12     0.068 ?   0.002  ops/us
RBTreeSearch.search:?gc.alloc.rate                    thrpt   12   259.208 ?   7.126  MB/sec
RBTreeSearch.search:?gc.alloc.rate.norm               thrpt   12  5024.473 ?   0.151    B/op
RBTreeSearch.search:?gc.churn.G1_Eden_Space           thrpt   12   249.292 ?  44.106  MB/sec
RBTreeSearch.search:?gc.churn.G1_Eden_Space.norm      thrpt   12  4834.329 ? 865.041    B/op
RBTreeSearch.search:?gc.churn.G1_Survivor_Space       thrpt   12     0.007 ?   0.015  MB/sec
RBTreeSearch.search:?gc.churn.G1_Survivor_Space.norm  thrpt   12     0.139 ?   0.284    B/op
RBTreeSearch.search:?gc.count                         thrpt   12    25.000            counts
RBTreeSearch.search:?gc.time                          thrpt   12    20.000                ms

and to compare with this fix:

Benchmark                                 Mode  Cnt  Score    Error   Units
RBTreeSearch.search                      thrpt   12  0.102 ?  0.003  ops/us
RBTreeSearch.search:?gc.alloc.rate       thrpt   12  0.002 ?  0.001  MB/sec
RBTreeSearch.search:?gc.alloc.rate.norm  thrpt   12  0.027 ?  0.001    B/op
RBTreeSearch.search:?gc.count            thrpt   12    ? 0           counts

As you can see, the footprint is quite different. It is almost zero, as expected for such a data structure using only primitive int, but after the fix. As Vladimir Kozlov pointed out, let me quote:

It shows that C2 did not eliminate Integer boxing allocations without your fix. As result they consume Java heap. With your fix C2's Escape Analysis eliminated these allocations.

From my point of view, this regression is a major issue that can improve the performance of a lot of applications, especially those that are sensitive to latency in general or the footprint in particular.

[catap]

@GoeLin any thought?

[bridgekeeper]

@catap This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[catap]

Thanks, bot! Any news?

[phohensee]

/sponsor

[openjdk]

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

• 2570e72: 8308300: enhance exceptions in MappedMemoryUtils.c
• ebb9a36: 8309297: Adjust ShenandoahHeap print_heap_regions_on
• 8ac9c57: Merge
• 0f531da: 8303465: KeyStore of type KeychainStore, provider Apple does not show all trusted certificates
• 22a548b: 8307395: Add missing STS to Shenandoah
• 12d08f0: 8308156: VerifyCACerts.java misses blank in error output
• e898048: 8307572: AArch64: Vector registers are clobbered by some macroassemblers
• f80be82: 8304976: Optimize DateTimeFormatterBuilder.ZoneTextPrinterParser.getTree()
• b6a5d20: 8269091: javax/sound/sampled/Clip/SetPositionHang.java failed with ArrayIndexOutOfBoundsException: Array index out of range: -4
• ef86ea2: 8283756: (zipfs) ZipFSOutputStreamTest.testOutputStream should only check inflated bytes
• ... and 105 more: https://git.openjdk.org/jdk17u-dev/compare/96adf073565e9a65912392510df8575e49695734...master

Your commit was automatically rebased without conflicts.

[openjdk]

@phohensee @catap Pushed as commit 6991372.

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