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

[Stefan Johansson]

> 12 nov. 2019 kl. 19:23 skrev Kim Barrett <kim.barrett at oracle.com>:
> 
>> On Nov 12, 2019, at 8:19 AM, Stefan Johansson <stefan.johansson at oracle.com> wrote:
>> 
>> Hi Kim,
>> 
>> Thanks for cleaning up this part of the code. Looks good overall but I have a couple of small comments below.
> 
> Thanks.
> 
>> 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?
> 
> The fanout from changing these counters from uint to size_t is pretty
> large.  It also doesn't help for a long-running 32bit application.
I see.

> 
> I thought about adding a utility for this kind of thing, since this isn’t the only occurrence.
> But I ran into problems for which I don’t currently have a solution (fixed by C++14).

Would a generic utility be implemented differently? To me this only works because we know that the values never differ much (1 or 2 I think), but I might be missing something.

Another approach would be to reset those values in a well defined manner to ensure they never wrap. From what I can see we only use them to ensure this state machine is doing what we expect, or is it possible that we can end up in a situation where we never could reset the values? 

> 
>> 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.
> 
> The overloading of "full" (concurrent full vs stw full) occurs in many
> places. But this particular area is using "old" rather than “full”, so
> I've changed the name.
> 
> New webrevs:
> full: https://cr.openjdk.java.net/~kbarrett/8232588/open.02/
> incr: https://cr.openjdk.java.net/~kbarrett/8232588/open.02.inc/

Looks good,
Stefan