RFR (M): 8040162: Avoid reallocating PLABs between GC phases in G1

[Mikael Gerdin]

Thomas,

On 2015-08-13 12:39, Thomas Schatzl wrote:
> Hi,
>
> On Wed, 2015-08-12 at 15:57 +0200, Mikael Gerdin wrote:
>> Hi Thomas,
>>
>> On 2015-08-12 12:38, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On Tue, 2015-08-11 at 16:56 +0200, Thomas Schatzl wrote:
>>>> Hi David,
>>>>
>>>> On Tue, 2015-08-11 at 15:17 +0200, David Lindholm wrote:
>>>>> Hi Thomas,
>>>>>
>>>>> Great work. Looks good, reviewed.
>>>>
>>>>     thanks for the review.
>>>
>>>     while testing the change a little more an issue with -XX:
>>> +ParallelRefProcEnabled has been found.
>>>
>>> Here is a new webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8040162/webrev.0_to_1/ (diff)
>>> http://cr.openjdk.java.net/~tschatzl/8040162/webrev.1/ (full)
>>
>> in g1CollectedHeap.cpp at
>> 4571       }
>> 4572
>> 4573       _root_processor->evacuate_roots(strong_root_cl,
>> 4574                                       weak_root_cl,
>> 4575                                       strong_cld_cl,
>> 4576                                       weak_cld_cl,
>> 4577                                       trace_metadata,
>> 4578                                       worker_id);
>> 4579
>> 4580       G1ParPushHeapRSClosure push_heap_rs_cl(_g1h, pss);
>> 4581       double start_strong_roots_sec = os::elapsedTime();
>> 4582       _root_processor->scan_remembered_sets(&push_heap_rs_cl,
>> 4583                                             weak_root_cl,
>> 4584                                             worker_id);
>> 4585       double strong_roots_sec = os::elapsedTime() -
>> start_strong_roots_sec;
>> 4586
>>
>> Shouldn't start_strong_roots_sec be taken before the call to
>> evacuate_roots? Otherwise it just measures the time it takes to scan the
>> remembered sets.
>>
>> 5400
>> 5401 // Weak Reference processing during an evacuation pause (part 1).
>> 5402 void
>> G1CollectedHeap::process_discovered_references(G1ParScanThreadState**
>> pss_) {
>> 5403   double ref_proc_start = os::elapsedTime();
>> 5404
>>
>> If you wan the local to be called pss, can you name the **
>> per_thread_states as it's named in the caller?
>>
>> in g1OopClosures.hpp
>> Would you consider moving set_ref_processor to G1ParScanClosure since
>> it's only used for that particular concrete type?
>> As I understand it it's only needed for the _scanner G1ParScanClosure
>> instance in the G1PSS that you need to be able to set the ref processor.
>>
>> Otherwise the change looks good to me.
>
> All fixed. Thanks particularly for catching the issue with the
> start_strong_roots_sec.
>
> Thanks for the review.
>
> Here are new webrevs.
>
> http://cr.openjdk.java.net/~tschatzl/8040162/webrev.1_to_2/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8040162/webrev.2/ (full)

Looks good.
/Mikael

>
> Thanks,
>    Thomas
>
>