RFR (M): 8008918: Reference statistics events for the tracing framework

Bengt Rutisson bengt.rutisson at oracle.com
Thu Mar 7 05:32:13 PST 2013


Hi Erik,

Looks good!

Bengt

On 3/6/13 6:06 PM, Erik Helin wrote:
> Hi Thomas,
>
> thanks for reviewing!
>
> Please see comments inline and an updated webrev located at:
> http://cr.openjdk.java.net/~ehelin/8008918/webrev.01/
>
> On 03/04/2013 09:24 AM, Thomas Schatzl wrote:
> > Hi,
> >
> > On Fri, 2013-03-01 at 15:09 +0100, Erik Helin wrote:
> >> Hi all,
> >>
> >> this change adds the vm/gc/reference/statistics event to all 
> collectors.
> >>
> >> Please note that this change depends on "8009232: Improve stats
> >> gathering code for reference processor" which is also out for 
> review on
> >> hotspot-gc-dev at openjdk.java.net.
> >>
> >> Webrev:
> >> http://cr.openjdk.java.net/~ehelin/8008918/webrev.00/
> >>
> >> Bug:
> >> https://jbs.oracle.com/bugs/browse/JDK-8008918
> >
> > Minor: Maybe, in concurrentMarkSweepGeneration.cpp the declaration of
> > the stats record could be put inside the block.
>
> Agree, updated.
>
> On 03/04/2013 09:24 AM, Thomas Schatzl wrote:
> > Do you think if there is an easy way to put the sequence of allocating
> > the stats record, calling rp->process_discovered_reference and the
> > report_gc_reference_processing into a single method that is called
> > everywhere?
>
> I don't think there is an easy way to do it :)
>
> Ideally I would like the ReferenceProcessor to take care of whether 
> the reference processing should be run in parallel or not. If this was 
> the case, then the code could have looked like the following everywhere:
>
>   const ReferenceProcessorStats& stats =
>     rp->process_discovered_references(...);
>   gc_tracer->report_gc_reference_stats(stats);
>
> On 03/04/2013 09:24 AM, Thomas Schatzl wrote:
> > This code sequence, is the same in all collectors, and seems to be a
> > good target for refactoring.
>
> Unfortunately, is is not exactly the same. Some collectors, for example
> ParNew, takes advantage of the fact that the caller checks whether the
> reference processing should be done in parallel or not:
>
>   if (rp->processing_is_mt()) {
>     ParNewRefProcTaskExecutor task_executor(*this, thread_state_set);
>     rp->process_discovered_references(&is_alive, &keep_alive,
>                                       &evacuate_followers, 
> &task_executor);
>   } else {
>     thread_state_set.flush();
>     gch->set_par_threads(0);  // 0 ==> non-parallel.
>     gch->save_marks();
>     rp->process_discovered_references(&is_alive, &keep_alive,
>                                       &evacuate_followers, NULL);
>   }
>
> If one refactor the code so that the ReferenceProcessor decides if the
> reference processing should run in parallel, then one has to ensure that
> the collector specific logic in the different branches still are
> correct.
>
> This is a cleanup that we definitely should do, but I would like to see
> it done as a separate change to ease the reviewing.
>
> On 03/04/2013 09:24 AM, Thomas Schatzl wrote:
> > I.e. what the change basically did is to replace the call to
> > process_discovered_reference by above sequence of three statements.
>
> I would say that the change made the code go from two to three lines.
> Before the code looked like:
>
>   if (rp->processig_is_mt()) {
>     rp->process...
>   } else {
>     rp->process...
>   }
> gc_tracer->report_gc_reference_processing(rp->collect_statistics());
>
> After the change, the code looks like:
>
>   ReferenceProcessorStats stats;
>   if (rp->processig_is_mt()) {
>     stats = rp->process...
>   } else {
>     stats = rp->process...
>   }
>   gc_tracer->report_gc_reference_processing(stats);
>
> The only difference in terms of lines is the addition of the "stats" 
> variable.
>
> In my opinion, this is a motivated cost for providing a more robust 
> way to collect the statistics from the reference processor. What do 
> you think?
>
> Thanks,
> Erik
>
> > Hth,
> > Thomas
> >
> >
>



More information about the hotspot-gc-dev mailing list