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

[John Cuthbertson]

Hi Thomas,

I think RegionTypeCounter should inherit from _ValueObj (by means of 
VALUE_OBJ_CLASS_SPEC).

Also a couple of nitty questions (feel free to ignore):

* Does the value parameter to RegionTypeCounter::percent_of need to be 
passed as a pointer? It looks like you pass the address of a field in 
the RegionTypeCounter instance and then dereference.
* For continuous humongous would it make sense to skip them until we 
process the StartsHumongous region and sum up the memory size and the 
occupied value for the entire humongous region (HS and HC) before adding 
to the humongous region counter?

Other than the above - it looks great!

Regards,

JohnC

On 6/4/2013 7:58 AM, Thomas Schatzl wrote:
> Hi all,
>
> On Tue, 2013-06-04 at 13:50 +0200, Bengt Rutisson wrote:
>> Hi Thomas,
>>
>> Thanks for fixing these things.
> A new webrev is up at
> http://cr.openjdk.java.net/~tschatzl/8014078/webrev.1/
>
> Changes:
> - spacing suggested by Jesper
> - removed comment in g1CollectedHeap::gc_prologue() indicating that the
> following line subtracted one because we are at the end of collection;
> both statements were just wrong.
> - remove G1SummarizeRSetStatsTime and print summaries both at start and
> end of collection
> - renamed region_type_counter_t to RegionTypeCounter and moved it in the
> top level scope
> - removed superfluous getters
> - renamed "Other" regions to "Old" regions in output
> - added some line spacing in G1HRRSStatsIter::print_summary_on()
> - refactoring of test cases:
>    - did _not_ use static imports because the java language does not
> allow them for the default package
>    - removed the testing for the G1SummarizeRSetStatsTime flag
>    - renamed of *Statistics() to *RSetSummaries()
>    - TestSummarizeRSetStats2 renamed to TestSummarizeRSetStatsPerRegion
>    - moved TestSummarizeRSetStatsPerRegion.expectStatistics to
> TestSummarizeRSetStatsTools.expectPerRegionRSetSummaries()
>    - adapted test for 8013895 to account for printing the summaries both
> at start and end of gc
>
>
> Testing:
> jtreg passed for this cr and 8013895, jprt still running.
>
>> 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,
>>> 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.
> I do not really mind.
>
>>> (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?
> Sure :)
>
>>>> 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.
> I hope too - if it isn't, just tell me.
>
>>>> parametrize the existing expectStatistics() method in there.
> I did not do that and kept two methods, although giving them to more
> distinct names.
>
> Thomas
>
>