On Wed, 25 Sep 2024 12:53:17 GMT, Roman Kennke <rken...@openjdk.org> wrote:
>> This is the main body of the JEP 450: Compact Object Headers (Experimental).
>>
>> It is also a follow-up to #20640, which now also includes (and supersedes)
>> #20603 and #20605, plus the Tiny Class-Pointers parts that have been
>> previously missing.
>>
>> Main changes:
>> - Introduction of the (experimental) flag UseCompactObjectHeaders. All
>> changes in this PR are protected by this flag. The purpose of the flag is to
>> provide a fallback, in case that users unexpectedly observe problems with
>> the new implementation. The intention is that this flag will remain
>> experimental and opt-in for at least one release, then make it on-by-default
>> and diagnostic (?), and eventually deprecate and obsolete it. However, there
>> are a few unknowns in that plan, specifically, we may want to further
>> improve compact headers to 4 bytes, we are planning to enhance the Klass*
>> encoding to support virtually unlimited number of Klasses, at which point we
>> could also obsolete UseCompressedClassPointers.
>> - The compressed Klass* can now be stored in the mark-word of objects. In
>> order to be able to do this, we are add some changes to GC forwarding (see
>> below) to protect the relevant (upper 22) bits of the mark-word. Significant
>> parts of this PR deal with loading the compressed Klass* from the mark-word.
>> This PR also changes some code paths (mostly in GCs) to be more careful when
>> accessing Klass* (or mark-word or size) to be able to fetch it from the
>> forwardee in case the object is forwarded.
>> - Self-forwarding in GCs (which is used to deal with promotion failure) now
>> uses a bit to indicate 'self-forwarding'. This is needed to preserve the
>> crucial Klass* bits in the header. This also allows to get rid of
>> preserved-header machinery in SerialGC and G1 (Parallel GC abuses
>> preserved-marks to also find all other relevant oops).
>> - Full GC forwarding now uses an encoding similar to compressed-oops. We
>> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB,
>> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the
>> GC forwarding at all).
>> - Instances can now have their base-offset (the offset where the field
>> layouter starts to place fields) at offset 8 (instead of 12 or 16).
>> - Arrays will now store their length at offset 8.
>> - CDS can now write and read archives with the compressed header. However,
>> it is not possible to read an archive that has been written with an opposite
>> setting of UseCompactObjectHeaders. Some build machinery is added so that
>> _co...
>
> Roman Kennke has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Allow LM_MONITOR on 32-bit platforms
src/hotspot/cpu/x86/macroAssembler_x86.cpp line 5692:
> 5690:
> 5691: void MacroAssembler::load_klass(Register dst, Register src, Register
> tmp) {
> 5692: BLOCK_COMMENT("load_klass");
I am not sure that the complexity of `MacroAssembler::load_klass` and the two
`MacroAssembler::cmp_klass` functions warrant adding block comments, but if you
prefer to leave them in, could you use opening and closing comments, as in the
other functions in this file (e.g. `MacroAssembler::_verify_oop`)? In that
case, please update the comment in the two `MacroAssembler::cmp_klass`
functions with a more descriptive name than `cmp_klass 1` and `cmp_klass 2`.
src/hotspot/cpu/x86/macroAssembler_x86.cpp line 5726:
> 5724: #ifdef _LP64
> 5725: if (UseCompactObjectHeaders) {
> 5726: load_nklass_compact(tmp, obj);
Suggestion: assert here that `tmp != noreg`, just like in
`MacroAssembler::cmp_klass(Register src, Register dst, Register tmp1, Register
tmp2)` below. Perhaps also assert that the input registers are different.
src/hotspot/cpu/x86/macroAssembler_x86.hpp line 379:
> 377: // Uses tmp1 and tmp2 as temporary registers.
> 378: void cmp_klass(Register src, Register dst, Register tmp1, Register
> tmp2);
> 379:
The naming of these two functions could be made clearer and more consistent
with their documentation. Please consider renaming the four-argument
`cmp_klass` function to `cmp_klasses_from_objects` or similar. The notion of
"source" and "destination" in the parameter names is unclear, I suggest to just
call them `obj`, `obj1`, `obj2` etc.
Please also make sure that the parameter names are consistent in the
declaration and definition (e.g. `dst` vs `obj`).
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 4008:
> 4006: #ifdef COMPILER2
> 4007: if ((UseAVX == 2) && EnableX86ECoreOpts && !UseCompactObjectHeaders) {
> 4008: generate_string_indexof(StubRoutines::_string_indexof_array);
This stub routine should be re-enabled if `UseCompactObjectHeaders` is to
become non-experimental and enabled by default in the future. Is there a RFE
for this task?
src/hotspot/share/opto/memnode.cpp line 1976:
> 1974: // The field is Klass::_prototype_header. Return its (constant)
> value.
> 1975: assert(this->Opcode() == Op_LoadX, "must load a proper type from
> _prototype_header");
> 1976: return TypeX::make(klass->prototype_header());
This code is dead, because by the time we call `load_array_final_field` from
`LoadNode::Value` (its only caller) we know that if `UseCompactObjectHeaders`,
then `tkls->offset() != in_bytes(Klass::prototype_header_offset()` (or else we
would have returned from line 2161). Please remove it, or replace it with an
assertion if you prefer.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1776676785
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1776628929
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1776644021
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1776663594
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1776621766
