JDK-8312018: Improve reservation of class space and CDS #15041
Conversation
|
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
tstuefe
force-pushed
the
JDK-8312018-Improve-odds-for-zero-base-optimized-reservation-of-class-space
branch
3 times, most recently
from
8738899 to
1df5f84
Compare
tstuefe
force-pushed
the
JDK-8312018-Improve-odds-for-zero-base-optimized-reservation-of-class-space
branch
from
f0b22e6 to
52a9810
Compare
tstuefe
force-pushed
the
JDK-8312018-Improve-odds-for-zero-base-optimized-reservation-of-class-space
branch
from
0d6b5eb to
381d249
Compare
tstuefe
force-pushed
the
JDK-8312018-Improve-odds-for-zero-base-optimized-reservation-of-class-space
branch
from
381d249 to
286a4c3
Compare
tstuefe
changed the title
|
Attention @iklam |
Webrevs
|
|
/issue JDK-8313669 |
|
@tstuefe |
There was a problem hiding this comment.
Some preliminary comments.
- I am not sure what "CDS" means in the PR title. It seems like CDS doesn't (yet) benefit from the improvements.
- I would suggest breaking this PR into smaller steps (see my comments inside
Metaspace::reserve_address_space_for_compressed_classes)
CDS uses the new API when reserving memory for the (by default now) relocated archive. So, while it not yet benefits from zero-based encoding, it benefits from the improved entropy the API gives. The old variant was using a randomized start position into an ordered list of 31+7 = 38 value points. The new API gives you entropy based solely on the alignment requirement for metaspace (16M). For unscaled, we have 255 possible attach points, for zero-based 1792. |
|
Hi @iklam, I addressed some of your concerns, please take another look. Thank you! |
Thanks, Ioi. I can add some ASCII art if needed. Tell me if its too complex for mere words :-) |
Many thanks, Roman! See answers inline. |
|
@iklam @rkennke thanks for the reviews so far. Changes to os::vm_min_address
|
…aarch64 /w 52 bit AS
|
@iklam is there anything missing from your point of view? |
I just realized this -- for the above 32GB allocations, do we need to use the new algorithm for all platforms? As far as I know, only aarch64 and ppc64 need it because they want to use a single "load immediate" instruction. For the other CPUs, we can just ask the OS. That will be faster, always succeed, and be at the "right" location as decided by the OS. |
The argument for doing it on the remaining platforms (x64 and risc) would be that those, too, could profit from using 16-bit moves and short immediates, instead of - e.g. in the case of x64 - always emitting a giant 8-byte immediate for addi. And that the code would be better tested, since all platforms run through it. OTOH, this could also be done in a follow-up. So, if you prefer it that way, I make that section aarch/ppc only. |
Do you mean this? I am not familiar with x64 instructions. I thought 64-bit immediate moves to a register must be 10 bytes (8 byte immediate value), if the value is larger than 32 bits. So you can't make the
For this PR, I would prefer doing it only on aarch64/ppc for the above 32GB allocations (otherwise we will have a regression for the other platforms -- there's now a chance of failure, at least theoretically). The algorithm is still used on all plaforms for the lower allocations, right? So we will get some test mileage that way. |
| // At CDS runtime, there is no need bothering with low-address reservation since we cannot set | ||
| // the encoding base to zero anyway: the encoding base has to be the base of the mapped archive | ||
| // (ultimately, the start address of the region we are reserving here). | ||
| const bool try_in_low_address_ranges = !cds_runtime; |
There was a problem hiding this comment.
I think try_in_low_address_range would be a far better name for the method param than either the current cds_runtime or your original strict_base, following the principle that it does what it says on the tin.
There was a problem hiding this comment.
Bike shedding done, you may stick with the current name if you wish.
There was a problem hiding this comment.
I like try_in_low_address_range, I will use that. Note that we hopefully can remove this clutch in the future.
src/hotspot/share/runtime/os.cpp
Outdated
| } | ||
| } | ||
|
|
||
| // Given an address range [min, max), attempts to reserve memory within this area, with the given alignmend. |
There was a problem hiding this comment.
... with the given alignment.
|
@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 13 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 |
Many thanks, @adinn! I massaged in your feedback. |
|
Thanks @iklam , @adinn and @rkennke! Also thanks to @TheRealMDoerr for testing at SAP. There is one remaining test issue on AIX, but I identified it as an old problem (https://bugs.openjdk.org/browse/JDK-8315321) and already opened a PR for it. For now, I disabled the test on AIX until that is fixed. I decided to push now, rather than after my vacation, to give this patch more time to test before 22 close. /integrate |
|
Going to push as commit 89d18ea.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
This PR cause compilation errors on Orange Pi5 Plus (http://www.orangepi.org/html/hardWare/computerAndMicrocontrollers/details/Orange-Pi-5-plus.html) The error message is as follows: |
| // we impose a minimum value if vm.mmap_min_addr is too low, for increased protection. | ||
| static size_t value = 0; | ||
| if (value == 0) { | ||
| assert(is_aligned(_vm_min_address_default, os::vm_allocation_granularity()), "Sanity"); |
There was a problem hiding this comment.
This line causes compilation error under Orange Pi5 Plus (http://www.orangepi.org/html/hardWare/computerAndMicrocontrollers/details/Orange-Pi-5-plus.html)
@wenshao Could you please test whether #16743 fixes your problem? If it still crashes, could you please reproduce the crash with and send the output? |
This PR rewrites memory reservation of class space and CDS to have ASLR and a much better chance of low-address placement.
Motivation:
(I was advised to keep PR text short lest I spam the mailing lists with Skara generated mails. So, for motivation, see JBS issue)
The patch introduces a new API to reserve memory within an address range at a randomized location, while trying to be smart about it. The API is generic, and future planned uses of this API could include replacing the zero-based heap allocation and the zero-based reservation of Shenandoah Collection Sets, thereby allowing us to consolidate coding.
This PR complements @iklam 's current work that rewrites archive heap initialization at runtime. Once his work is in, we will be able to recalculate narrow Klass IDs for objects loaded from the archive, and that will allow us to reap the benefits of this patch for the CDS runtime case too.
Noteworthy functional changes:
os::vm_min_address()Example (linux amd64):
We start the JVM with a 30GB heap.
In the stock JVM, the JVM will place the heap in the lower address ranges starting at 2G (0x8000_0000). But then it is unable to place the class space in lower regions too, so it placed it at 32 GB (0x8_0000_0000), and we don't have zero-based encoding (Narrow klass base: 0x0000000800000000). This scenario repeats for every iteration, so we will always use these two addresses (no ASLR):
In the patched JDK, the heap will also be placed at 2G (0x8000_0000). But the class space will be nestled below that, at a random location lower than 2G (in this run, at 0x1f00_0000):
And it will be randomized, see output from multiple runs - every time we end up with a zero-base, but the actual start address of the class space differs:
Tests:
The patch comes with a full set of gtests that test the new API from all angles, all of which have been executed on many platforms.
GHAs are green, too.
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15041/head:pull/15041$ git checkout pull/15041Update a local copy of the PR:
$ git checkout pull/15041$ git pull https://git.openjdk.org/jdk.git pull/15041/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15041View PR using the GUI difftool:
$ git pr show -t 15041Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15041.diff
Webrev
Link to Webrev Comment