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

[Dmitry Fazunenko]

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)


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

>
> - line 145, it should read "Object*s* per region"
fixed
>
> - line 146, please write "Heap *fraction* to allocate" since there is
> enough space and it is clearer.
fixed
>
> - in the three paragraphs that are printed at startup, the ":" are
> aligned differently within the paragraphs, please use a common style in
> the output.
fixed
>
> - 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();

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

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

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)*----------


Thanks,
Dima

>
> Thanks,
>    Thomas
>