New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JDK-8294677: chunklevel::MAX_CHUNK_WORD_SIZE too small for some applications #12115
JDK-8294677: chunklevel::MAX_CHUNK_WORD_SIZE too small for some applications #12115
Conversation
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Thomas,
Looks good to me.
Thanks for fixing this quickly!
@tstuefe 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:
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 39 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm.
Test errors seem unrelated. SAP nightlies are green (three days). I ran a number of tests manually to check that the memory footprint envelope does not change. |
/integrate |
Going to push as commit 2292ce1.
Your commit was automatically rebased without conflicts. |
This patch is a workaround for a regression caused by the JEP-387 metaspace revamp. That revamp introduced a maximum limit to the size an individual metaspace allocation could have (4m).
4m was deemed much more than enough; normally metaspace allocations are tiny. But we now had a customer trying to load a generated giant class. The class was inefficiently generated but valid nonetheless.
The patch increases the maximum chunk size in Metaspace from 4M to 16M. This fixes the customer case mentioned in the JBS note. However, this is just a workaround since we still have a limit, albeit a larger one. Therefore I am working on a prototype that removes the limit completely (https://bugs.openjdk.org/browse/JDK-8300729) but that needs some more cooking time.
For details, please see JBS text as well as JDK-8300729.
Patch increases the maximum chunk size.
This affects the granularity used to reserve (not: commit - no RSS increase, just address space) metaspace memory, which in turn affects CompressedClassSpaceSize granularity (it gets silently rounded up to a multiple of root chunk size) as well as class space placement. Thankfully all of this is well abstracted behind
Metaspace::reserve_alignment()
so almost no changes were necessary. The increase from 4m to 16m is also small enough to be benign.Increasing the maximum chunk size also increases the memory and runtime footprint of some tests that explicitly test metaspace chunk management. I checked all of these tests and changed some to keep the footprint low.
Finally, the patch uncovered some minor test errors in metaspace tests:
The gtest "metaspace:get_chunk_with_commit_limit" was supposed to test allocation from a metaspace chunk when hit by a commit limit. The limit was too high, effectively disabling the commit-failed path of this test. I expanded the test to test a variation of commit limits, not only one. The test also produced wrong positives with
MetaspaceReclaimPolicy=none
- those are fixed now (see comment). Note that I plan to remove theMetaspaceReclaimPolicy
switch since, in practice, it seems of very limited use, and this would cut down on test variations and code complexity.The gtest "metaspace:misc_sizes" tests that we can allocate the max. root chunk size (abstracted via
Metaspace::max_allocation_word_size
). A larger root chunk size caused us to hit the GC threshold now. I adjusted the test and expanded it to test both class space and non-class metaspace.The elastic metaspace jtreg tests (test/hotspot/jtreg/runtime/Metaspace/elastic...) needed adaption since they have the root chunk size hard-coded. There is also a minor issue with using word size, but I left that untouched and opened https://bugs.openjdk.org/browse/JDK-8300732 to deal with this later.
Tests:
I manually ran hotspot_metaspace tests (a collection of metaspace, gtests, cds tests and some GC tests) on x64 and x86, fastdebug and release. GHAs are in process, and more tests are planned.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12115/head:pull/12115
$ git checkout pull/12115
Update a local copy of the PR:
$ git checkout pull/12115
$ git pull https://git.openjdk.org/jdk pull/12115/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12115
View PR using the GUI difftool:
$ git pr show -t 12115
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12115.diff