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

[Bengt Rutisson]

Hi Thomas,

Thanks for fixing this so quickly!

Looks good.

About the check for UseG1GC on the command line. I see your point and 
I'm fine with leaving it in. We really don't have a good way of 
filtering test for specific collectors. This is one way of doing it so I 
guess it's fine. I hope we soon figure out how to do this in the more 
general case.

Thanks,
Bengt

On 9/25/13 2:06 PM, Thomas Schatzl wrote:
> 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
>
>