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

[Bengt Rutisson]

Hi Thomas,

Long overdue, but here's a review of this.

Overall it looks really good. Thanks for fixing this!

Some minor comments:

g1RemSetSummary.cpp

  256     _all.add(rs_mem_sz, occ, code_root_mem_sz, code_root_elems);
  257     if (r->is_young()) {
  258       _young.add(rs_mem_sz, occ, code_root_mem_sz, code_root_elems);
  259     } else if (r->isHumongous()) {
  260       _humonguous.add(rs_mem_sz, occ, code_root_mem_sz, 
code_root_elems);
  261     } else if (r->is_empty()) {
  262       _free.add(rs_mem_sz, occ, code_root_mem_sz, code_root_elems);
  263     } else {
  264       _old.add(rs_mem_sz, occ, code_root_mem_sz, code_root_elems);
  265     }

It is difficult to quickly see if the same arguments are passed to all 
counters. I think I would prefer this instead:

   RegionTypeCounter* current;
   if (r->is_young()) {
     current = &_young;
   } else if (r->isHumongous()) {
     current = &_humongous;
   } else if (r->is_empty()) {
     current = &_free;
   } else {
     current = &_old;
   }
   current->add(rs_mem_sz, occ, code_root_mem_sz, code_root_elems);
   _all.add(rs_mem_sz, occ, code_root_mem_sz, code_root_elems);


Also, I think occ is too short to be a good name. Would prefer occupied.

I would prefer to not have the macro FOR_ALL_REGION_COUNTERS. Better to 
inline the code or refactor it in some other way I think.


The test:

RunSystemGCs is a bit of a strange name for the class. Could we come up 
with a more descriptive name?

Since the test anyway spawns a separate process with the correct 
arguments we don't really need this check, right?

   43         if (!TestSummarizeRSetStatsTools.testingG1GC()) {
   44             return;
   45         }

The test also seems to assume that we run with compressed oops enabled 
on 64 bit machines. I guess that will always be the case sine you have 
-Xmx20m but maybe we should comment on that somewhere or explicitly set 
it on the command line.

Thanks,
Bengt

On 9/3/13 12:05 PM, Thomas Schatzl wrote:
> Hi all,
>
>    can I have reviews for the following change? It has already been
> reviewed earlier
> (http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2013-June/007431.html), but not pushed at that time because of concerns of introducing bugs so late in the jdk8 release process.
>
> Changes in the code base have shown that this has not been a
> particularly good idea, as this requires lots of work; since the change
> itself is extremely low risk, includes test cases and provides useful
> information, so we think it is okay to push.
>
> This re-review is caused by mentioned changes: particularly the nmethod
> changes (CR 7145569) added a new kind of remembered set entries which
> need to be handled by the output.
>
> This CR has the following effects:
>
> - changes the output of -XX:+G1SummarizeRSetStats to contain a breakdown
> of remembered set data structures by region type, i.e. young, humongous,
> old, and free regions.
>
> The resulting output looks as follows (just a -version run):
>
>   Cumulative RS summary
>
>   Recent concurrent refinement statistics
>    Processed 0 cards
>    Of 0 completed buffers:
>              0 (  0.0%) by concurrent RS threads.
>              0 (  0.0%) by mutator threads.
>    Did 0 coarsenings.
>    Concurrent RS threads times (s)
>            0.00     0.00     0.00     0.00
>    Concurrent sampling threads times (s)
>            0.00
>
>   Current rem set statistics
>    Total per region rem sets sizes = 531K. Max = 2K.
>             2K (  0.5%) by 1 Young regions
>             0K (  0.0%) by 0 Humonguous regions
>           528K ( 99.5%) by 183 Free regions
>             0K (  0.0%) by 0 Old regions
>     Static structures = 91K, free_lists = 0K.
>      0 occupied cards represented.
>              0 (  0.0%) entries by 1 Young regions
>              0 (  0.0%) entries by 0 Humonguous regions
>              0 (  0.0%) entries by 183 Free regions
>              0 (  0.0%) entries by 0 Old regions
>      Region with largest rem set = 0:(E)[0x0000000748e00000,0x0000000748e580e0,0x0000000748f00000], size = 2K, occupied = 0K.
>    Total heap region code root sets sizes = 20K.  Max = 0K.
>             0K (  0.5%) by 1 Young regions
>             0K (  0.0%) by 0 Humonguous regions
>            20K ( 99.5%) by 183 Free regions
>             0K (  0.0%) by 0 Old regions
>      0 code roots represented.
>              0 (  0.0%) elements by 1 Young regions
>              0 (  0.0%) elements by 0 Humonguous regions
>              0 (  0.0%) elements by 183 Free regions
>              0 (  0.0%) elements by 0 Old regions
>      Region with largest amount of code roots = 183:(F)[0x0000000754500000,0x0000000754500000,0x0000000754600000], size = 0K, num_elems = 0.
>
> - cleanup of output to be more readable (i.e. CR 8023253)
>
> - 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.
>
> - all calculated memory size values are rounded downwards, instead of
> sometimes up and sometimes down as earlier; I could not find a clear pattern
> so I decided to always round down but if there is some reason to do
> otherwise let me know.
>
> bugs.sun.com
> http://bugs.sun.com/view_bug.do?bug_id=8014078
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8014078/webrev.4/
>
> Testing:
> jprt, jtreg test case for 8014078 and 8013895
>
> Hth,
>    Thomas
>