RFR: 8057586: Explicit GC ignored if GCLocker is active [v5]

[Albert Mingkun Yang]

On Tue, 18 Apr 2023 09:35:52 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

>> Hi All,
>> 
>> Please review this change to guarantee that at least a Full GC is executed between the invocation and return of `System.gc` or `WhiteBox.fullGC`, even if the call is concurrent with an active `GCLocker `. 
>> 
>> The change should also handle the issues reported in JDK-8299276.
>> 
>> Split into 3 commits, one commit for changes to each GC in [G1, Parallel, Serial]. 
>> 
>> Testing: Tier 1-5.
>
> Ivan Walulya has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Thomas Review

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 556:

> 554:     VMThread::execute(&op);
> 555: 
> 556:     if (!VM_ParallelGCSystemGC::is_cause_full(cause) || op.gc_succeeded()) {

`!is_cause_full` would cover more cases than `System.gc` and whitebox-fullgc, right? Is this really intended?

The introduce of `op.gc_succeeded()` is not well motivated -- the semantics of the return-val of `invoke()` is also not obvious at first glance. Therefore, I'd prefer keeping the existing signature.

test/hotspot/jtreg/gc/TestJNICriticalStressTest.java line 175:

> 173:         long durationSec = Long.parseLong(args[0]);
> 174:         int allocThreadNum = Integer.parseInt(args[1]);
> 175:         int jniCriticalThreadNum = Integer.parseInt(args[2]);

Why is this always one in all test cases? Wouldn't it be more "stressing" to use sth larger? Same as `allocThreadNum` for instance.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13191#discussion_r1169951304
PR Review Comment: https://git.openjdk.org/jdk/pull/13191#discussion_r1169864279