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

[Thomas Schatzl]

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)

Thanks,
  Thomas