RFR: 8323273: AArch64: Strengthen CompressedClassPointers initialization check for base

[Thomas Stuefe]

On Tue, 16 Jan 2024 02:41:40 GMT, Yude Lin <duke at openjdk.org> wrote:

> Summary:
> Add a platform-dependent check for CompressedClassSpaceBaseAddress;
> Remove the "reserve anywhere" attempt after the initial mapping attempt failed---this is rarely used and will likely fail anyway, because the accepted mapping is very restricted on aarch64;
> Additional assertions after initialization.
> 
> Passed hotspot/jtreg/:tier1 on fastdebug

Hi @linade,

as part of Lilliput, we are planning larger changes to this code in mainline - see https://github.com/openjdk/lilliput/pull/128, which will also go in a bit different form into mainline. It would be nice if you could hold off your PR until then.

But I am also not convinced the changes are needed. I see some improvements to code clarity, but those I would rather see after Lilliput hits, in a separate RFE. For the two functional main points this PR addresses:

- `CompressedClassSpaceBaseAddress` is a development debug-only tool that should do exactly what's on the label: dumbly enforce an address. Its only purpose is to observe the JVM's handling of this address. If that handling includes asserting, I'd like to see that. There is no need for more complexity - it is a simple tool with sharp edges and it only exists for tests.
- I don't see what we gain by removing the "reserve anywhere" path. "It may fail with a high probability" is not enough reason - it also may succeed. At the cost of one additional mmap call (which we probably called before that point a hundred times) it still can prevent an early VM death.

Cheers, Thomas

src/hotspot/cpu/aarch64/compressedKlass_aarch64.cpp line 70:

> 68:     const int index = (start_index + ntry + alt_index) % num_immediates;
> 69:     const uint64_t immediate = ((uint64_t)immediates[index]) << 32;
> 70:     assert(immediate > 0 && MacroAssembler::eor_compatible_klass_encoding(immediate) >= 32,

The format of this helper is rather awkward. It returns 0 if the address is not compatible? Otherwise, the number of trailing zeros?

I think there is no strong need for any of this, since the original assert is already over-cautious (after all, we only test a range of hard-coded values). But if you like to add a test for the lower 32-bit, I'd just add that here, no need for these functions.

src/hotspot/cpu/aarch64/compressedKlass_aarch64.cpp line 134:

> 132:   _range = end - _base;
> 133: 
> 134:   DEBUG_ONLY(assert_is_valid_encoding(addr, len, _base, _shift);)

asserts are debug only, DEBUG_ONLY is not needed

src/hotspot/cpu/aarch64/compressedKlass_aarch64.cpp line 142:

> 140:           (1ULL << MacroAssembler::eor_compatible_klass_encoding((uint64_t)base) >= range) ||
> 141:           MacroAssembler::movk_compatible_klass_encoding((uint64_t)base)) &&
> 142:          (shift == -1 /* Not specified */ || shift == 0);

This duplicates the assert we have in MacroAssembler; why do this twice, in two places?

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 4682:

> 4680:     (uint64_t)CompressedKlassPointers::base() >> CompressedKlassPointers::shift();
> 4681:   guarantee(movk_compatible_klass_encoding(shifted_base),
> 4682:             "compressed class base bad alignment");

I also think this code can benefit from a bit cleanup some time in the future, but for now I ask you to hold off: there is no pressing need at the moment, and once Lilliput things have settled, this code should be easier to work with it. 

Also, there are a few more things we could improve, and I rather see those in a combined single RFE.

src/hotspot/share/memory/metaspace.cpp line 590:

> 588: 
> 589: #if !defined(AARCH64)
> 590:   // On aarch64, this likely wouldn't satisfy the encoding constraints.

Please don't add platform ifdefs here! We put much work into getting rid of platform-dependent sections. Also, as stated above, I think this change is not needed.

-------------

PR Review: https://git.openjdk.org/jdk/pull/17437#pullrequestreview-1872611278
PR Review Comment: https://git.openjdk.org/jdk/pull/17437#discussion_r1484464819
PR Review Comment: https://git.openjdk.org/jdk/pull/17437#discussion_r1484451905
PR Review Comment: https://git.openjdk.org/jdk/pull/17437#discussion_r1484466475
PR Review Comment: https://git.openjdk.org/jdk/pull/17437#discussion_r1484470480
PR Review Comment: https://git.openjdk.org/jdk/pull/17437#discussion_r1484446574