RFR: 8300915: G1: incomplete SATB because nmethod entry barriers don't get armed

[Thomas Schatzl]

On Wed, 25 Jan 2023 13:32:27 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

> The change makes sure all nmethod entry barriers are armed when a G1 concurrent marking cycle starts by doing it unconditionally in `G1ConcurrentMark::pre_concurrent_start()`. The barrier is needed to add oop constants to the SATB.
> 
> This alone would be a conservative/minimal fix but I didn't like that `CodeCache::is_gc_marking_cycle_active()` can return true after a marking cycle start is undone. I.e. `G1ConcurrentMarkThread::_state == Idle && CodeCache::is_gc_marking_cycle_active()` is possible.
> Edit: It felt like an inconsistency but maybe its actually ok not to finish the CC marking cycle when the G1 marking is undone. Please comment...
> The changes in `G1ConcurrentMark::post_concurrent_undo_start()` were made to avoid it.
> 
> 
> Testing
> 
> * Rebased this PR onto [JDK-8297487](https://bugs.openjdk.org/browse/JDK-8297487), replaced related assertions with guarantees, and then executed test/langtools:tier1 in a loop for 12h without seeing the issues reported in [JDK-8299956](https://bugs.openjdk.org/browse/JDK-8299956).
> 
> * CI testing at SAP: This includes most JCK and JTREG tiers 1-4, also in Xcomp mode, on the standard platforms and also on ppc64le.
> 
> * GHA: build failures because download of apache ant failed. `serviceability/jvmti/vthread/SuspendResumeAll/SuspendResumeAll.java` had a timeout on windows-x64. Should be unrelated. It succeeded in our CI testing.

Changes requested by tschatzl (Reviewer).

>  Edit: It felt like an inconsistency but maybe its actually ok not to finish the CC marking cycle when the G1 marking is undone. Please comment...
> The changes in G1ConcurrentMark::post_concurrent_undo_start() were made to avoid it.

I think this has merely been an oversight/bug.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 3376:

> 3374:   } else {
> 3375:     assert(static_cast<G1CollectedHeap*>(Universe::heap())->_cm->has_aborted(),
> 3376:            "expected stw full gc after cm has aborted");

Why not:

Suggestion:

    assert(G1CollectedHeap::heap()->concurrent_mark()->has_aborted(),
           "Expected full gc after concurrent mark has aborted");

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 796:

> 794: 
> 795:   CodeCache::on_gc_marking_cycle_start();
> 796:   CodeCache::arm_all_nmethods();

Maybe factor this out into a method (`start_codecache_marking_cycle`) and call it above.

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

PR: https://git.openjdk.org/jdk/pull/12194