RFR: 8141278: New tests for PLAB testing

[Thomas Schatzl]

Hi Michail,

On Tue, 2015-11-17 at 19:28 +0300, Michail Chernov wrote:

  sorry for the very long waiting...

That will unfortunately also cause some problems because in the
meantime we moved to Unified logging

> Hello,
> 
> Could I have a couple of reviews for the new PLAB tests please?
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8141278
> Webrev: http://cr.openjdk.java.net/~mchernov/8141278/webrev.00/
> 
> There are two new tests for PLAB.

General comments:

- would it help if the PLAB logging would print out what generation the
particular output is for? That would probably make the code easier to
understand than the current way of filtering out every second printout
(and potentially decrease the size of LogParser a lot).

Also, unified logging should help you getting only the messages you are
interested in.

> 
> Test TestPLABNoResize checks how PLAB is used for promotion of
> objects. 
> It starts  the AppPLABFixedSize and inspects GC log details. The 
> AppPLABFixedSize tries to fill specified amount of memory with small 
> object, initiates young GC twice to promote objects from eden to 
> survivor and then from survivor to old gen. TestPLABNoResize has 
> different scenarios:
> 1. Promotion of small objects, which should be promoted using PLAB.
> 2. Promotion of big objects, which should be promoted by direct
> allocation.
> 3. Unreachable objects should not be promoted.

TestPLABNoResize.java

- the @summary description should be more concise, e.g. change
@summary: "Test for PLAB allocation in Survivor and Old" to "Test that
PLABs are never resized if PLAB resizing is disabled."

- line 65: the comment does not tell what the threshold is used for.
E.g. instead of "some threshold" use "Threshold to determine whether
the correct amount of objects were promoted. This is only an
approximate threshold for these checks."

Actually I am not sure whether it is useful to all these checks with
the threshold, because it seems to be very large.

- line 144: I think "No dead object should be promoted." reads better

- line 145+: if we really need to check the correct amount of promoted
bytes (not sure if this is useful here), also check that the sums of
plabAllocated and directAllocated are below that threshold.

- maybe the TestCase class could (but possibly should not?) be merged
in some way that the TestCase for PLABResize seems to be a part of the
TestCase for PLABNoResize

> 
> Test TestPLABResize check behaviour of PLAB resizing. Test executes
> the 
> AppPLABResize which tries to do next:
> 1. Performs some iterations of creating some fixed amount of objects
> and 
> promoting them to survivor. Promotion of fixed amount of objects will
> cause to fixed desired PLAB size.
> 2. Performs some iterations - creates some amount of object, promotes
> it 
> to survivor, decreases amount of objects. It causes to decreasing of 
> desired PLAB size.
> 3. Performs next iterations which is similar to step 2, but increases
> amount of object to get increasing of desired PLAB size.
> 

There are a few typos in the strings, "previous" is written as
"previout".

- please rename AppPLABFixedSize to AppPLABNoResize to avoid some
confusion about what "application" is part of which test.

- both in AppPLABFixedSize and AppPLABNoResize: "A simple application a
part of PLAB no resize test." -> "This application is part of the PLAB
[no] resize test".

AppPLABFixedSize:

- Instead of:

  40      * AppPLABFixedSize for testing fixed size PLAB.
  41      * Expects next properties: chunk.size, allocator.type,
reachable

Maybe:

"Used for testing that there is no PLAB resizing. Expects the following
properties to be set:
  chunk.size some-explanation
  ...
"

PLABUtils.java

  37      * List of VM options for GC tuning.

-> "PLAB tests default option list."

  56      * List of options for WhiteBox using.

-> "List of options required to use WhiteBox."

Storage.java

  26  * The Storage is used for storing reachable objects.
  27  * Class will store not more than capacity, ...
  28  * If we exceed capacity, object will be stored at existing
entries
  29  * So, if capacity=1, all previously added objects will be
unreachable

The Storage class ...
This class will not store than a given number of objects...
If its capacity is exceeded, old objects will be overwritten.
E.g. if capacity is one, only the last object added will be saved.


> Both tests use LogParser to get VM log information.
> 
> Issues which were found by these tests:
> https://bugs.openjdk.java.net/browse/JDK-8140585 - PLAB statistics
> are 
> flushed too late
> https://bugs.openjdk.java.net/browse/JDK-8141141 - Young and Old gen 
> PLAB stats are similar in output with -XX:+PrintPLAB
> https://bugs.openjdk.java.net/browse/JDK-8139903 - G1EvacStats does
> not 
> split log entries.


Thanks,
  Thomas