RFR: 8240774: [REDO] G1DirtyCardQueue destructor has useless flush

[David Holmes]

On Wed, 24 May 2023 10:24:56 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

> Please review this change to VM initialization, to eliminate a workaround for
> a bootstrapping problem.
> 
> A JavaThread must not touch oops and invoke barriers unless it is on the
> threads list.  Barrier set operations on_thread_attach and on_thread_detach
> bracket the time when a thread can do stuff with oops.
> 
> However, this restriction is currently violated by the main thread during VM
> initialization.  It may (and indeed does) touch oops before it has been added
> to the thread list, including executing post-barriers and enqueuing entries
> in its DCQ.
> 
> That has led to two separate bootstrapping workarounds:
> 
> - ~G1DirtyCardQueue flushes the queue before proceeding to the base class
> (PtrQueue) destructor. This is not normally needed, and is even
> counter-productive, since it may hide a problem. But if an error is detected
> during VM initialization that main thread gets deleted. Since it's not on the
> threads list, there's no on_thread_detach to flush the queue. Without the
> flush in the DCQ destructor that can lead to an assert in ~PtrQueue because
> the queue isn't empty.
> 
> - on_thread_attach can't assert the queue is empty, even though normally it
> must be.  But for the main thread, being added to the threads list (so calling
> on_thread_attach) happens after it has enqueued entries in its DCQ.  So we
> currently don't have those asserts.
> 
> To remove the need for these workarounds, this change moves the addition of
> the main thread to the threads list earlier in the initialization sequence. It
> needs to be before code that might execute barriers. After init_globals is too
> late. But it can't be too early, before some relevant data structures are
> initialized, so it can't be before init_globals. So this change splits
> init_globals at a strategic point, and adds the main thread to the threads
> list there. That point is just before the call to universe2_init, which calls
> Universe::genesis.
> 
> With that change we can then remove the workarounds in ~G1DirtyCardQueue and
> G1BarrierSet::on_thread_attach.
> 
> Testing:
> mach5 tier1-5
> 
> Manually tested error handling for failures reported by init_globals and
> init_globals2. This involved patching to force error reporting when
> -XX:+UseNewCode. Handling errors from init_globals works as before, if a
> failure happened in that part of the sequence. Handling errors from
> init_globals2 runs afoul of pre-existing JDK-8308764.

Can't really comment on the workaround removal, but the change to the init sequence looks okay.

Thanks.

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14119#pullrequestreview-1443202138