RFR (S/M): 8014078: G1: improve remembered set summary information by providing per region type information

[Bengt Rutisson]

Hi Thomas,

Thanks for fixing these things.

Some comments inline.

On 6/4/13 1:39 PM, Thomas Schatzl wrote:
> Hi,
>
> On Tue, 2013-06-04 at 08:10 +0200, Bengt Rutisson wrote:
>> Hi Thomas,
>>
>> First, a couple of questions about the CR. It is a P4 enhancement and we
>> have passed feature complete. Should this still go in to hs25? If yes,
>> could you update the CR a bit with why this extra remembered set
>> information is important. Just so we understand why it needs to go in
>> now. Also, I think it would be good if the CR mentions that the
>> remembered set information is now printed before GC as well as after.
> Updated.
>
>> Some code comments:
>>
>> g1CollectedHeap.cpp.
>>
>> This comment needs to be updated since we are _before_ gc:
>>
>> 3542       // we are at the end of the GC. Total collections has already
>> been increased.
> Fixed. Forgot to update the comment.
>
>> I don't really like the flag G1SummarizeRSetStatsTime. I would prefer
>> two separate flags, G1SummarizeRSetStatsBeforeGC and
>> G1SummarizeRSetStatsAfterGC. Or maybe just one flag that always prints
>> before and after GC, similar to PrintHeapAtGC.
> I was thinking about that too but rejected it due to backwards
> compatibility. What to do about G1SummarizeRSetStats after introducing
> the Before/AfterGC flags?
>
> It seems awkward to need both G1SummarizeRSetStats and
> G1SummarizeRSetStatsBeforeGC/G1SummarizeRSetStatsAfterGC. What are the
> rules on removing diagnostic flags?

I think I vote for only having G1SummarizeRSetStats and let that print 
the information both before and after GC. That is backwards compatible 
in the sense that you get at least the information you got before with 
this flag. It is also similar to the PrintHeapAtGC flag.

>
> (Note that I think that G1SummarizeRSetStats has had a lot of use -
> before cr 8013895 its output has been very buggy).

I think you mean that it has _not_ had a lot of use, right?

>
>> g1RemSetSummary.cpp
>>
>> Why is region_type_counter_t a struct and not a class? I would prefer it
>> to be a class and be called something like RegionTypeCounter. Also I
>> think it would be nicer to declare it outside of HRRSStatsIter.
> Fine with me. Done.
>
>> I think the getters for young(), humongous(), free() and other() are
>> unnecessary. I would use the variables directly since I think the
>> getters introduce a lot of line noise in this class. If you want to keep
>> the getters I would suggest to make them one-liners similar to the other
>> getters in the class, so:
>>
>> region_type_counter_t& young() { return _young; }
> Removed the getters, in this case I agree on second thought.
>
>> Also, isn't the "other" category actually "old" regions? If yes, would
>> you mind changing the name of them to "old"?
> G1 surprisingly does not have an explicit notion of old (and free, but
> there's at least a single predicate to determine that) regions, old is
> just what is left if you look at the if-then-else cascades in the code
> (not only in these changes).
>
> That's the reason for this name. I changed it though.
>
>> The method print_summary_on() is very hard to read. Can you try to
>> format it in a way that makes it more readable? I think just adding a
>> few line breaks between the different section would help a lot.
>>
> Done.
>
>> I have not looked too closely at the tests but some comments:
>>
>> In TestSummarizeRSetStats.java you could minimize the diff and make the
>> test more readable by using static imports for
>> TestSummarizeRSetStatsTools.runTest() and
>> TestSummarizeRSetStatsTools.expectStatistics()
> I did not know that language feature... thanks! :)
>
>> I think the name TestSummarizeRSetStats2 is a bit awkward. How about
>> TestSummarizeRSetStatsPerRegion?
> Done.
>
>> The method TestSummarizeRSetStats2.expectStatistics() is a bit
>> surprising since the other tests use
>> TestSummarizeRSetStatsTools.expectStatistics(). Could we move
>> TestSummarizeRSetStats2.expectStatistics() into
>> TestSummarizeRSetStatsTools too? Either with a new name or we could
> I will clean this up - but I would like to have your comment on the flag
> issue first. Also, I will then send a new webrev.

Sounds like a good plan. I hope what I wrote above is enough.

Thanks,
Bengt

>
>> parametrize the existing expectStatistics() method in there.
> Thanks,
>
> Thomas
>
>