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

[Bengt Rutisson]

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.

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.


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.


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.

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

Also, isn't the "other" category actually "old" regions? If yes, would 
you mind changing the name of them to "old"?

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.


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 think the name TestSummarizeRSetStats2 is a bit awkward. How about 
TestSummarizeRSetStatsPerRegion?

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 
parametrize the existing expectStatistics() method in there.

Thanks,
Bengt


On 5/28/13 10:26 AM, Thomas Schatzl wrote:
> Hi all,
>
>    could I get some reviews for the following change? It improves the
> remembered set summary information available with -XX:
> +G1SummarizeRSetStats by adding memory size and occupancy summaries
> broken down by region type (young, humonguous, free, other).
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8014078/webrev/
>
> Note that this webrev is based on 8013895; I provided a full changeset
> including 8013895 at
> http://cr.openjdk.java.net/~tschatzl/8014078/webrev/8014078.8013895.diff
>
> JIRA:
> https://jbs.oracle.com/bugs/browse/JDK-8014078
>
> bugs.sun.com:
> http://bugs.sun.com/view_bug.do?bug_id=8014078
>
> Testing:
> jprt, jtreg tests
>
> About the change:
>
> - the output of G1SummarizeRSetStats now looks as follows (for
> dacapo/eclipse):
>
> After GC RS summary
>
>   Concurrent RS processed 15299 cards
>    Of 64 completed buffers:
>             64 (100.0%) by concurrent RS threads.
>              0 (  0.0%) by mutator threads.
>    Concurrent RS threads times (s)
>            0.06
>    Concurrent sampling threads times (s)
>            0.00
>    Total heap region rem set sizes = 512K.  Max = 13K.
>>>>>       17K (  3.4%) by 7 Young regions
>>>>>        7K (  1.4%) by 3 Humonguous regions
>>>>>      157K ( 30.6%) by 64 Free regions
>>>>>      330K ( 64.6%) by 44 Other regions
>    Static structures = 1K, free_lists = 254K.
>      277592 occupied cards represented.
>>>>>         0 (  0.0%) entries by 7 Young regions
>>>>>         2 (  0.0%) entries by 3 Humonguous regions
>>>>>         0 (  0.0%) entries by 64 Free regions
>>>>>    277590 (100.0%) entries by 44 Other regions
>      Max size region =
> 3:(O)[0x00000000f0900000,0x00000000f0a00000,0x00000000f0a00000], size =
> 14K, occupied = 25K.
>      Did 0 coarsenings.
>
> Notice the breakdown of memory size and card occupancy by region type
> (marked with >>>> here).
>
> "Young" regions include all types of young regions (i.e. survivor) as
> survivor does not have a meaning for g1 outside of a gc.
> "Humonguous" and "Free" are self-explaining. "Other" regions include
> everything else.
>
> - added a new flag G1SummarizeRSetStatsTime that is used to control the
> time, i.e. before/after gc, when the periodicaly summaries are printed.
> This integer is treated as a bitset, where a set first bit indicates a
> request to print a summary after gc, the second bit a request to print a
> summary before gc.
>
> The alternative (which I'd like more) would be to change
> G1SummarizeRSetStats to act like this new flag to avoid introducing a
> new flag; however I'm hesitant to change this as G1SummarizeRSetStats is
> a publicly available flag (although diagnostic).
>
> - cleanup of the test cases: a large part of the code base for this CR
> reuses utility methods from 8013895, so I went ahead and factored out
> common code into a separate file that both the tests for 801385 and this
> CR use.
>
> Thanks,
> Thomas
>