RFR: 8254623: gc/g1/TestHumongousConcurrentStartUndo.java still fails sometimes [v2]

[Kim Barrett]

On Thu, 15 Oct 2020 14:26:23 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hi all,
>> 
>>   can I have reviews for this change of the gc/g1/TestHumongousConcurrentStartUndo.java test to make it even more
>>   resilient to timing?
>> 
>> Previously the test tried to create a stable initial condition by performing a full gc before that test step, but the
>> problem is about when the first concurrent mark happens while building up the large set of humongous object kept alive:
>> if it happens too early, and the concurrent undo operation takes too long, the next concurrent start will just be
>> another undo because the large set of humongous objects will already be unreferenced and may as well cause a concurrent
>> undo and not a concurrent mark.  The solution I came up with is that *after* creating all the humongous objects of the
>> large set trigger an extra concurrent mark (and wait for its completion). That one (or some before) must have been a
>> concurrent mark because at least at that point the heap must have been occupied by more than the IHOP value.  Testing:
>> 2k iterations on failing platform (always aarch64-linux) without issues.  Thanks,
>>   Thomas
>
> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   kbarrett review

Changes requested by kbarrett (Reviewer).

test/hotspot/jtreg/gc/g1/TestHumongousConcurrentStartUndo.java line 108:

> 106:         }
> 107:
> 108:         private static void runConcurrentUndoCycle(ArrayBlockingQueue a) {

The queue argument ("a") is unused.

test/hotspot/jtreg/gc/g1/TestHumongousConcurrentStartUndo.java line 127:

> 125:                 System.out.println("Acquire CM control");
> 126:                 WB.concurrentGCAcquireControl();
> 127:                 WB.concurrentGCRunToIdle();

RunToIdle is not needed here.  AcquireControl implicitly does that.

test/hotspot/jtreg/gc/g1/TestHumongousConcurrentStartUndo.java line 137:

> 135:             // in "a" which is larger than the IHOP threshold. Another dummy humongous
> 136:             // allocation must trigger a concurrent cycle that is not an Undo Cycle.
> 137:             allocateHumongous(1, new ArrayBlockingQueue(1));

Since "a" is not referenced beyond this point, I think it is at least theoretically possible that it, and the objects
it references, could be treated as dead, allowing an Undo Cycle here too. A following Reference.reachabilityFench(a)
could be used to guarantee the desired lifetime.

Doing so also suggests making "a" local rather than an argument, making its size locally obvious.

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

PR: https://git.openjdk.java.net/jdk/pull/632