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

[Thomas Schatzl]

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?

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

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

> parametrize the existing expectStatistics() method in there.

Thanks,

Thomas