RFR(M): 8248401: Refactor/unify RMI gc support functionality

[Thomas Schatzl]

Hi Albert,

   thanks for your review.

On 07.08.20 12:22, Albert Yang wrote:
> Hi,
> 
> The patch looks good to me; some small comments/suggestions.
> 
> The comment for `millis_since_last_whole_heap_examined` seems incorrect; 
> it should be "the whole heap" not "any patr of the heap".
> ```
>    // Returns the longest time (in ms) that has elapsed since the last
>    // time that any part of the heap was examined by a garbage collection.
> ```

You are right. I meant it as "all parts of the heap".

> 
> The name, `next_whole_heap_examined`, doesn't sound quite right. (It's 
> not obvious to me what that function does from its name.) Maybe 
> `record_whole_heap_examined_timestamp`?

Done.

> 
> I think it's best to call `next_whole_heap_examined` at the end of 
> `PSParallelCompact::post_compact()`, just like before this patch.

Fine with me.

> 
> Maybe we can preserve this commment in the declaration of 
> `_last_whole_heap_examined_time_ns` so that readers know we choose `ns` 
> on purpose, even though the caller, `JVM_MaxObjectInspectionAge`, only 
> need `ms`.
> ```
>    // We need to use a monotonically non-decreasing time in ms
>    // or we will see time-warp warnings and os::javaTimeMillis()
>    // does not guarantee monotonicity. > ```

I added a comment, but left the specifics about os::javaTimeMillis() out 
there. First, it would be better placed at the implementation, and 
second we are using the nanosecond counter anyway. Talking about a 
method that returns milliseconds seems confusing.

New webrevs:

http://cr.openjdk.java.net/~tschatzl/8248401/webrev.1_to_2 (diff)
http://cr.openjdk.java.net/~tschatzl/8248401/webrev.2 (full)

Testing: local with new test

Thanks,
   Thomas

> 
> On 2020-08-04 12:02, Thomas Schatzl wrote:
>> Hi Stefan,
>>
>>   thanks for your review.
>>
>> On 03.08.20 13:30, Stefan Karlsson wrote:
>>> Hi Thomas,
>>>
>>> Not a complete review, but regarding the ZGC change, I don't think 
>>> it's appropriate to add a callback to Universe/CollectedHeap to 
>>> ZStatCycle. Most functions in ZStat only collects and print 
>>> statistics, and does not trigger external events. I think moving it 
>>> to the call would be cleaner:
>>>
>>>    ~ZDriverGCScope() {
>>>     ...
>>>      // Update statistics
>>>      ZStatCycle::at_end(_gc_cause, boost_factor);
>>>
>>>      // Update data used by soft reference policy
>>>      Universe::update_heap_info_at_gc();
>>>
>>>      // Maybe add the call here?
>>>      ...
>>>    }
>>>
>>
>> Thank you for pointing out the appropriate place in ZGC. Done.
>>
>>> Seeing this, it's also apparent that some of the "last_gc" info has 
>>> historically been recorded in Universe:
>>>
>>> void Universe::update_heap_info_at_gc() {
>>>    _heap_capacity_at_last_gc = heap()->capacity();
>>>    _heap_used_at_last_gc     = heap()->used();
>>> }
>>>
>>> while _last_whole_heap_examined_time_ns is recorded in CollectedHeap. 
>>> I think we should create a RFR to move the heap variables out of 
>>> Universe.
>>
>> Done, JDK-8250961.
>>
>> In addition to above changes, I tried to move the new call closer to 
>> Universe::update_heap_info_at_gc() in a few places and improve naming 
>> of some local variables.
>>
>> New webrevs:
>> http://cr.openjdk.java.net/~tschatzl/8248401/webrev.0_to_1 (diff)
>> http://cr.openjdk.java.net/~tschatzl/8248401/webrev.1 (full)
>>
>> Reran the test 300 times with these changes.
>>
>> Thanks,
>>   Thomas
>