RFR: 8134963 [Newtests] New tests for G1 remembered set up and down

[Dmitry Fazunenko]

Thank you very much Thomas for the all comments provided!

-- Dima

On 27.01.2016 15:21, Thomas Schatzl wrote:
> Hi Dima,
>
> On Tue, 2016-01-26 at 19:36 +0300, Dmitry Fazunenko wrote:
>> Hi Thomas,
>>
>> Many thanks for your comments!
>>
>> New version is available here:
>> http://cr.openjdk.java.net/~dfazunen/8134963/webrev.04/
>>
>> The comments are inline:
>>
>> On 14.01.2016 15:49, Thomas Schatzl wrote:
>>> Hi Dima,
>>>
>>> On Wed, 2015-12-30 at 19:43 +0300, Dmitry Fazunenko wrote:
>>>> Hi everyone,
>>>>
>>>> An updated version of the stress test for Remembered Set:
>>>>       http://cr.openjdk.java.net/~dfazunen/8134963/webrev.03/
>>>>
>>>> Tested in RBT in many configurations.
>>>> Many thanks to Thomas for his offline comments.
>>>     some comments:
>>>
>>> - I would prefer to have described the input parameters somewhere
>>> in
>>> the description of the test (the "what the test does" one) instead
>>> of
>>> some comments in the main() method.
>>    * Test Parameters:
>>    *   args[0] - number of objects per Heap Region (1 - means
>> humongous)
>>    *   args[1] - number of regions to refresh to provoke GC at the end
>> of
>> cycle.
>>    *             (0 - means no GC, i.e. no reading from RSet)
>>    *   args[2] - timeout in seconds (to stop execution to avoid jtreg
>> timeout)
> Okay.
>
>
>>> - is there some way to add some kind of progress indicator?
>> done: output looks like
>> %% step 6 out of 10 (~33% done)
>> %%      0  --> 256
>> %% step 7 out of 10 (~65% done)
>> %%      256  --> 266
>> %% step 8 out of 10 (~66% done)
>> %%      266  --> 5
> Okay.
>
>>> - line 200-202: the thresholds are automatically calculated, based
>>> on region size and options. See
>>> OtherRegionsTable::OtherRegionsTable for the fine->coarse
>>> threshold (_max_fine_entries), and SparsePRT::cards_num() for an
>>> entry point for the sparse->fine threshold.
>>>
>>> Maybe they should be retrieved from the corresponding VM global.
>> fixed
>>           // threshold for sparce -> fine
>>           final int FINE =
>> WB.getIntxVMFlag("G1RSetSparseRegionEntries").intValue();
>>
>>           // threshold for fine -> coarse
>>           final int COARSE =
>> WB.getIntxVMFlag("G1RSetRegionEntries").intValue();
> Thanks.
>
>>> - maybe an explanation what regToRefCounts is would be nice. Note
>>> that I think it is (slightly) useful to actually add more than
>>> COARSE entries (a few) to check that code path a bit more (adding
>>> entries to rsets that are already coarsened).
>>>
>>> - a bit more documentation in go() what is actually done.
>>>
>>> E.g. maybe this
>>>
>>> // an object from "to" region
>>> // if pre > cur, then we will forget
>>>
>>> would read better as
>>>
>>> // Select an object that we will install references to. If the
>>> number
>>> // of references after should be less than they were before, select
>>> // NULL.
>> done.
>>
>>> - while I typically do not comment on naming of local variables, I
>>> have no clue what "star" should mean in this context. Maybe
>>> "target" or just "obj" would make my life happier :)
>> star --> celebrity
>>
>> the celebrity is a representative of a region, which become very
>> popular among others
>>
>>                       // Select a celebrity object that we will
>> install
>> references to.
>>                       // The celebrity will be referred from all other
>> regions.
>>                       // If the number of references after should be
>> less
>> than they
>>                       // were before, select NULL.
> Just "target" without the long comment would have been fine, but now
> that you already described the variable name, you may as wel keep it :)
>
>>> - please start comments with upper case and ending with a full stop
>>> (if they are full sentences).
>> done
>>> - not sure what the "reading" means, probably you mean "To force
>>> the
>>> use of rememebered set entries we need to provoke a GC. To induce
>>> some
>>> fragmentation, and some mixed GCs, we need to make a few objects
>>> unreachable.".
>> done
> Thanks.
>
>>> - as we already discussed in email, the stress test times out on
>>> slow machines (I think these machines are always old SPARC with
>>> very few threads). I am not sure what is better, either decrease
>>> the length of the test or the timeout. (Did you try on e.g. Intel
>>> Atoms? - not sure if they qualify since the heap requirement for
>>> the test is 3G min)
>>>
>>> But an alternative could be some regular polling of the current
>>> runtime in the application and exit early.
>>>
>>> I fear that otherwise we will spend more time on this in the
>>> future.
>> Done. Test stops working if the given timeout has achieved:
> Thanks a lot for this.
>
>> 539.845: #30: [GC pause (WhiteBox Initiated Concurrent Mark) (young)
>> (initial-mark) 925M->924M(960M), 0.3904663 secs]
>> 540.235: #31: [GC concurrent-mark-start]
>> 541.547: #31: [GC concurrent-mark-end, 1.3114741 secs]
>> 541.547: #31: [GC remark, 0.0081503 secs]
>> 541.555: #31: [GC cleanup 924M->924M(960M), 0.0428539 secs]
>> %% step 6 out of 10 (~33% done)
>> %%      0  --> 256
>> %% step 7 out of 10 (~65% done)
>> %%      256  --> 266
>> %% step 8 out of 10 (~66% done)
>> %%      266  --> 5
>> %% TIMEOUT!!!
>> %% Summary
>> %%   Time spent          : 1620 seconds
>> %%   Free memory left    : 35840K
>> %% Test passed
>> ----------System.err:(1/15)----------
>> STATUS:Passed.
>> ----------rerun:(23/2223)*----------
> Some other typos:
>
>    76  *   from Rx to Ry. If c[i] < c[i-1] at the end of iteration
>
> HTML encoded "<" sign :)
>
>   221         // regToRegRefCounts - array of reference counts from
> region to region.
>
> The full stop should be removed as the sentence seems to be continued
> in the next line.
>
>   223         // The number of test iterations is array legnth - 1.
>
> length
>
>   226         // If c[i] < c[i-1] then some referenes will be cleaned.
>        
>
> references
>
>   267                     //Number of references went down.
>   268                     //Need to provoke recalculation of RSet.
>
> Missing spaces after the "//"
>
> Given that my remaining issues are about typos, I do not need a re
> -review for fixing them.
>
> Also thanks for renaming the test (and the CR).
>
> Looks good.
>
> Thanks,
>    Thomas
>
>