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

[Albert Yang]

Thank you; the new patch looks good to me.

On 2020-08-07 13:22, Thomas Schatzl wrote:
> 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
>>
>
-- 
/Albert