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

[Thomas Schatzl]

Hi,

On Wed, 2013-09-25 at 12:53 +0200, Bengt Rutisson wrote:
> Hi Thomas,
> 
> Long overdue, but here's a review of this.

Thanks.

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

All fixed. Renamed "occ" to "occupied_cards".

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

Inlined. There are several options to refactor, but they would just add
more code bloat imo.

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

Changed to VerifySummaryOutput.

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

This will avoid running the test if the original command line does not
contain UseG1GC, i.e. G1 is not tested.

First, this is intended to avoid crashes if the VM this test runs does
not support G1, and second it avoids unnecessary runs of the same test.
I.e. I expect that the test is run with different collectors on the same
machine. I do not see a point in running this test if the user did not
want G1 testing.

Sort of second-guessing the intent of the tester, decreasing the amount
of runs, and avoiding issues with testing on platforms not supporting
g1.

I can remove it though.

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

Added to command line.

New webrev at
http://cr.openjdk.java.net/~tschatzl/8014078/webrev.5/

Thomas