RFR: 8344363: FullGCForwarding::initialize_flags is called after ObjLayout::initialize

[Stefan Karlsson]

On Tue, 19 Nov 2024 20:07:38 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

> From the bug description:
> ObjLayout::initialize() is called in Arguments::parse(const JavaVMInitArgs*) which sets ObjLayout::_klass_mode. FullGCForwarding::initialize_flags(size_t) is called in init_globals() which seems to be later in the Threads::create_vm(JavaVMInitArgs*, bool*) routine. The latter, however, can unset the UseCompactObjectHeaders flag, which leads to a potential mismatch with ObjLayout::_klass_mode, firing the asserts in ObjLayout::klass_mode().
> 
> There is also a related (and somewhat minor) problem: In Arguments::parse() we enable a bunch of stuff when UseCompactObjectHeaders is on (LW locking, but that's the default anyway, and object-monitor-tables, which are otherwise off), but then may disable UseCompactObjectHeaders, e.g. in the GC or when -UseCompressedClassPointers is requested. This then leaves the odd situation that we run without compact headers, but have object monitor tables turned on.
> 
> The fix to both issues is:
> - First disable UCOH
> - and do that in apply_ergo() (e.g. GCs should do it in GCArguments::initialize())
> - then enable any flags required by compact headers
> - Initialize ObjLayout after all flags are done (it's in the correct place, already)
> 
> Testing:
>  - [x] tier1 -UCOH (default)
>  - [ ] tier1 +UCOH
>  - [x] Manual testing several flag combinations

I think this looks good. I ran it through gdb with awatch to see that I there was no obvious reads of the variables before they were set.

A couple of small nits that would be nice fix.

src/hotspot/share/gc/serial/serialArguments.cpp line 32:

> 30: 
> 31: void SerialArguments::initialize() {
> 32:   GenArguments::initialize();

Suggestion:

  GCArguments::initialize();

There's no `GenArguments::initialize()` so I think it would be clearer to call the function with the parent class instead. Alternatively, make `ParallelGC::initialize` also call `GenArguments::initialize`.

src/hotspot/share/gc/serial/serialArguments.hpp line 35:

> 33: private:
> 34:   virtual CollectedHeap* create_heap();
> 35:   virtual void initialize();

All other GC's put the `initialize` function before the `create_heap`. It might be nice to keep that consistent.

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

Marked as reviewed by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22255#pullrequestreview-2448960364
PR Review Comment: https://git.openjdk.org/jdk/pull/22255#discussion_r1850523493
PR Review Comment: https://git.openjdk.org/jdk/pull/22255#discussion_r1850529411