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

[Jesper Wilhelmsson]

Looks good!
Ship it!
/Jesper

On 6/3/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
>  >
>  >
>