RFR (M): 6672778: G1 should trim task queues more aggressively during evacuation pauses

[Stefan Johansson]

On 2018-04-13 10:35, Thomas Schatzl wrote:
> Hi Stefan,
> 
>    thanks for your review... :)
> 
> On Thu, 2018-04-12 at 17:15 +0200, Stefan Johansson wrote:
>> Hi Thomas,
>>
>> On 2018-04-11 13:46, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     I updated and (hopefully) improved the change a bit after some
>>> more thinking.
> [...]
>>>
>>> Webrevs:
>>> http://cr.openjdk.java.net/~tschatzl/6672778/webrev.0_to_1/ (diff)
>>> http://cr.openjdk.java.net/~tschatzl/6672778/webrev.1/ (full)
>>
>> Thanks for this update, I had a glance at the first version and I
>> think this is indeed simpler. Still have some comments though:
>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
>> 3129 root_processor->evacuate_roots(pss, pss->closures(), worker_id);
>>
>> Only pass in pss, and get the closures inside evactuate_roots.
>> -----
>> src/hotspot/share/gc/g1/g1SharedClosures.hpp
>> 48 double trim_time_sec() {
>> 49   return TicksToTimeHelper::seconds(_oops.trim_ticks_and_reset())
>> +
>> 50     TicksToTimeHelper::seconds(_oops_in_cld.trim_ticks_and_reset()
>> );
>> 51 }
>>
>> This could be removed now, right?
>> -----
>>
>> Apart from this me and Thomas has also spoken a bit offline so there
>> might be some additional changes.
>>
>> Thanks,
>> Stefan
> 
>    all done.
> 
> http://cr.openjdk.java.net/~tschatzl/6672778/webrev.1_to_2/ (diff)
> http://cr.openjdk.java.net/~tschatzl/6672778/webrev.2/ (full)

I like this :) Some additional small comments:

src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp
  516     _start_time += TicksToTimeHelper::seconds(_trim_time);

Maybe add a comment to explain when we update _start_time, something like:
// The trim_time should be excluded from this phase, do this by updating
// the start time.
-----

src/hotspot/share/gc/g1/g1GCPhaseTimes.hpp
  380 class G1GCParPhaseTimesTracker : public CHeapObj<mtGC> {

Why do you need to change this to be CHeapObj instead of StackObj?
-----

src/hotspot/share/gc/g1/g1ParScanThreadState.hpp

   64   uint const _stack_drain_upper_threshold;
   65   uint const _stack_drain_lower_threshold;

As you know I like "drain" better than "trim" but to be consistent I 
think we should name these _queue_trim_xxxxx_threshold. Or go the other 
way and name everything "drain" =)
------

Thanks,
Stefan

> 
> Thanks,
>    Thomas
>