RFR: 8293824: gc/whitebox/TestConcMarkCycleWB.java failed "RuntimeException: assertTrue: expected true, was false" [v2]

[Ivan Walulya]

On Tue, 6 Dec 2022 12:34:47 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Please review this change to WhiteBox and some tests involving G1 concurrent GCs.
>> 
>> Some tests currently use WhiteBox.g1StartConcMarkCycle() to trigger a
>> concurrent GC.  Many of them follow it with a loop waiting for a concurrent
>> cycle to not be in progress.  A few also preceed that call with a similar
>> loop, since that call does nothing and returns false if a concurrent cycle is
>> already in progress.  Those tests typically want to ensure there was a
>> concurrent cycle that was started after some setup.
>> 
>> The failing test calls that function, asserting that it returned true, e.g. a
>> new concurrent cycle was started.
>> 
>> There are various problems with this, due to races with concurrent cycles
>> started automatically and possibly aborted (by full GCs) concurrent cycles,
>> making some of these tests unreliable in some configurations.
>> 
>> For example, the test failure associated with this bug intermittently arises
>> when running with `-Xcomp`, triggering a concurrent cycle before the explicit
>> requests by the test, causing the explicit request to fail (because there is
>> already one in progress), failing the assert.  Waiting for there not to be an
>> in-progress cycle before the explicit request just narrows the race window.
>> 
>> We have a different mechanism for controlling concurrent cycles, the
>> concurrent GC breakpoint mechanism.  By adding a counter specifically for such
>> cycles, we can use GC breakpoints to ensure only the concurrent cycles the
>> test wants are occurring, and can verify they completed successfully.
>> 
>> So we change tests using WhiteBox.g1StartConcMarkCycle() to instead use GC
>> breakpoints, along with the new WhiteBox.g1CompletedConcurrentMarkCycles() to
>> avoid racing request problems and to detect aborted cycles.  Since it is no
>> longer used, WhiteBox.g1StartConcMarkCycle() is removed.
>> 
>> Testing:
>> mach5 tier1-6
>
> Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits:
> 
>  - use new whitebox api for running conc gc
>  - Merge branch 'master' into test-concmark
>  - copyrights
>  - remove WB.g1StartConcMarkCyle
>  - update tests
>  - add utility GC breakpoint functions
>  - add G1ConcurrentMark::completed_mark_cycles() and whitebox access

Lgtm!

Minor comments:

test/hotspot/jtreg/gc/g1/humongousObjects/objectGraphTest/GCTokens.java line 36:

> 34:     public static final String WB_INITIATED_YOUNG_GC = "Young (Normal) (WhiteBox Initiated Young GC)";
> 35:     public static final String WB_INITIATED_MIXED_GC = "Young (Mixed) (WhiteBox Initiated Young GC)";
> 36:     public static final String WB_INITIATED_CMC = "WhiteBox Initiated Run to Breakpoint";

`WB_INITIATED_CMC = "WhiteBox Initiated Run to Concurrent GC Breakpoint"; ` as we indicate below that `String CMC = "Concurrent Mark)"`

test/hotspot/jtreg/gc/stress/TestStressRSetCoarsening.java line 296:

> 294:                     // Number of references went down.
> 295:                     // Need to provoke recalculation of RSet.
> 296:                     WB.g1RunConcurrentGC(false);

probably use `WB.g1RunConcurrentGC();` overload for consistency with other calls

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

Marked as reviewed by iwalulya (Reviewer).

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