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

Thomas Schatzl thomas.schatzl at oracle.com
Tue Jun 4 14:58:33 UTC 2013


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





More information about the hotspot-gc-dev mailing list