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

Albert Yang albert.m.yang at oracle.com
Fri Aug 7 10:22:13 UTC 2020


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.
```

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`?

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

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.
```

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



More information about the hotspot-gc-dev mailing list