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

[Erik Helin]

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
 >
 >