RFR: 8231153: Improve concurrent refinement statistics

[Stefan Johansson]

On 2019-10-08 00:38, Kim Barrett wrote:
>> On Oct 7, 2019, at 8:57 AM, Stefan Johansson <stefan.johansson at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> On 2019-10-02 03:08, Kim Barrett wrote:
>>> New webrevs:
>>> full: https://cr.openjdk.java.net/~kbarrett/8231153/open.01/
>>> incr: https://cr.openjdk.java.net/~kbarrett/8231153/open.01.inc/
>>
>> The changes looks good, just one question around the calculation of total time and size.
>>
>> src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp
>> ---
>> 415 Tickspan G1ConcurrentRefine::total_refinement_time() const {
>> ...
>> 425   const_cast<G1ConcurrentRefine*>(this)->threads_do(&closure);
>> 426   return closure._total_time;
>> 427 }
>> 428
>> 429 size_t G1ConcurrentRefine::total_refined_cards() const {
>> ...
>> 439   const_cast<G1ConcurrentRefine*>(this)->threads_do(&closure);
>> 440   return closure._total_cards;
>> 441 }
>>
>> Did you consider grouping these two functions into one, to avoid iterating the threads twice? Not sure this is a big deal, and it might only make the code more complicated, but it feels a bit unnecessary to do two iteration right after each other.
> 
> Thanks for the suggestion. I tried doing something like that in an
> earlier version of this change, but I didn't like how it turned out.
> But enough code has changed since then that I decided to try again.
> This time seems okay.
> 
> So G1ConcurrentRefine now provides a RefinementStats class that
> packages up the time and card counts, and a new function
> total_refinment_stats() that returns one of those. Also removed
> total_refinement_time() and total_refined_cards(), which are no longer
> used. (If that were to change they are easily reinstated as wrappers
> over total_refinement_stats().)
> 
> New webrevs:
> full: https://cr.openjdk.java.net/~kbarrett/8231153/open.02/
> incr: https://cr.openjdk.java.net/~kbarrett/8231153/open.02.inc/
>
Thanks for trying it out a second time, this is more or less exactly 
what I had in mind.

Looks good,
Stefan

> Testing:
> mach5 tier1
> 
>