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

[Mikael Gerdin]

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.

/Mikael

>
> Thanks,
>    Thomas
>
>