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

[Thomas Schatzl]

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