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

[Dmitry Fazunenko]

Hi Thomas,

Thanks a lot, that you found time to look that code. Appreciate it very 
much.
Please see the comments inline

On 20.11.2015 13:41, Thomas Schatzl wrote:
> Hi Dmitry,
>
> On Fri, 2015-11-13 at 18:15 +0300, Dmitry Fazunenko wrote:
>> Hi everyone,
>>
>> Can I have a couple of reviews for the new test for G1 Remembered Set:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8134963
>> http://cr.openjdk.java.net/~dfazunen/8134963/
>>
>> The test creates a lot of cross region references (making RSet to grow
>> up) and then clear that references up.
>> The test doesn't check any particular assertions, it just expects that
>> there will be no crash.
> Some comments:
>
>    - the test does not stress the remembered set operations well:
>
>       - runtime is too short. In my test runs, not a single remembered
> set cleanup occurs, i.e. the remembered set will never shrink.
May be I need to play with a build where a new remsets is implemented.
My assumption was that the new remsets implementation tracks not only 
adding cross region references, but their removing as well:

objectInRegion1.field = objectInRegion2;  // region1 ---> region2
objectInRegion1.field = null;                       // region1 -X-> 
region2  (if there is no other references)

Perhaps, my assumption is wrong. In this case, which event should 
trigger rescan of the remembered set?
In my test there are no allocations during up and down phases, therefore 
no GC.

>
>       - in general, due to the short runtime, g1 seems to be warming up.

The test allocates ~90% of heap. Is it still not enough to warm up?

>
>       - the test does not stress the need for remsets being MT safe at
> all. I.e. the main program uses one thread.
Valid point. I think it should another (similar) test using MT.


>
>    - accidentally the test has been run in some jprt jobs. I got a few
> out-of-native memory errors running it (that may be due to some other
> changes in that build though).
The test is moved to test/stress/
   http://cr.openjdk.java.net/~dfazunen/8134963/webrev.02/
To not be run in jprt.

Maybe the fact that it caught out-of-native memory errors proves that 
the test is good, because it catches issues?


>
> So I think while the test may be correct, it just does not seem to do a
> good job on its task.
>
> I frequently use a test based on a contribution years ago (well,
> basically a verbatim copy) when working on remembered sets (see the
> attachment at
> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2012-July/004740.html), running it for a few minutes. Of course, it is a lot more demanding than the test in this change. Maybe it can be scaled back to a useful size, and potentially the object size used there varied a little.

Do you think that the test (TestUpAndDownRSet) is totally useless? Or it 
makes sense to implement enhancement you suggested?

I looked at the BigRamTester you mentioned. It looks like a regular test 
consuming a certain amount of heap. It's not clear to me why this test 
is going to stress a new RemSet.

Thanks,
Dima



>
> Thanks,
>    Thomas
>
>