RFR: 8232588: G1 concurrent System.gc can return early or late

[Stefan Johansson]

Hi Kim,

Thanks for cleaning up this part of the code. Looks good overall but I 
have a couple of small comments below.

On 2019-11-11 20:40, Kim Barrett wrote:
>> On Nov 7, 2019, at 4:02 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>
>>> On Nov 7, 2019, at 4:58 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>>>> New webrevs:
>>>> full: https://cr.openjdk.java.net/~kbarrett/8232588/open.01/

src/hotspot/share/gc/g1/g1CollectedHeap.cpp
---
2089 // Return true if (x < y) with allowance for wraparound.
2090 static bool gc_counter_less_than(uint x, uint y) {
2091   return (x - y) > (UINT_MAX/2);
2092 }

This code makes me have to think to much, with regards to what it does. 
What do you think about using size_t instead of uint and just do simple 
comparisons?
---

src/hotspot/share/runtime/mutexLocker.hpp
---
71 extern Monitor* G1FullGCCount_lock;              // in support of 
"concurrent" full gc

I think the name and the comment could be improve now when it's G1 
specific. Something like G1OldGCCount_lock, to me Full signals that it's 
only STW.
---

Thanks,
Stefan

>>>> incr: https://cr.openjdk.java.net/~kbarrett/8232588/open.01.inc/
>>>
>>> Looks good (obviously missing the items discussed here).
> 
> After sending out the open.01* set of webrevs I noticed the comment
> describing should_do_concurrent_full_gc had fallen out of date a while
> ago (should have been updated by JDK-8212657), and could also use a
> bit of tidying up.  I'd like to deal with this now, since it's minimal
> and I'm waiting on a 2nd reviewer.
> 
> Changes are to add missing _g1_periodic_collection to the list and
> regularize punctuation there, and improve wording of the description.
> Not bothering with a webrev for now; here's the diff:
> 
> diff -r f95bdf58fc7e -r a772f9ce0594 src/hotspot/share/gc/g1/g1CollectedHeap.hpp
> --- a/src/hotspot/share/gc/g1/g1CollectedHeap.hpp	Wed Nov 06 18:52:16 2019 -0500
> +++ b/src/hotspot/share/gc/g1/g1CollectedHeap.hpp	Mon Nov 11 14:36:22 2019 -0500
> @@ -256,14 +256,14 @@
>   
>     G1HRPrinter _hr_printer;
>   
> -  // It decides whether an explicit GC should start a concurrent cycle
> -  // instead of doing a STW GC. Currently, a concurrent cycle is
> -  // explicitly started if:
> -  // (a) cause == _gc_locker and +GCLockerInvokesConcurrent, or
> -  // (b) cause == _g1_humongous_allocation
> -  // (c) cause == _java_lang_system_gc and +ExplicitGCInvokesConcurrent.
> -  // (d) cause == _dcmd_gc_run and +ExplicitGCInvokesConcurrent.
> -  // (e) cause == _wb_conc_mark
> +  // Return true if an explicit GC should start a concurrent cycle instead
> +  // of doing a STW full GC. A concurrent cycle should be started if:
> +  // (a) cause == _gc_locker and +GCLockerInvokesConcurrent,
> +  // (b) cause == _g1_humongous_allocation,
> +  // (c) cause == _java_lang_system_gc and +ExplicitGCInvokesConcurrent,
> +  // (d) cause == _dcmd_gc_run and +ExplicitGCInvokesConcurrent,
> +  // (e) cause == _wb_conc_mark,
> +  // (f) cause == _g1_periodic_collection and +G1PeriodicGCInvokesConcurrent.
>     bool should_do_concurrent_full_gc(GCCause::Cause cause);
>   
>     // Attempt to start a concurrent cycle with the indicated cause.
>