RFR: 8262049: [TESTBUG] Shenandoah: Adjustments in TestReferenceRefersTo.java for IU mode

[Roman Kennke]

Shenandoah's IU mode allows referents to be cleared even when accessed during concurrent marking. The test TestReferenceRefersTo.java needs to be adjusted to allow for that.

Test:
 - [x] TestReferenceRefersTo.java + Shenandoah/IU
 - [x] TestReferenceRefersTo.java + Shenandoah/SATB
 - [x] TestReferenceRefersTo.java + G1

-------------

Commit messages:
 - 8262049: [TESTBUG] Shenandoah: Adjustments in TestReferenceRefersTo.java for IU mode

Changes: https://git.openjdk.java.net/jdk/pull/2653/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2653&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8262049
  Stats: 18 lines in 1 file changed: 12 ins; 0 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2653.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2653/head:pull/2653

PR: https://git.openjdk.java.net/jdk/pull/2653

[Kim Barrett-2]

On Fri, 19 Feb 2021 19:48:51 GMT, Roman Kennke <[hidden email]> wrote:

> Shenandoah's IU mode allows referents to be cleared even when accessed during concurrent marking. The test TestReferenceRefersTo.java needs to be adjusted to allow for that.
>
> Test:
>  - [x] TestReferenceRefersTo.java + Shenandoah/IU
>  - [x] TestReferenceRefersTo.java + Shenandoah/SATB
>  - [x] TestReferenceRefersTo.java + G1

Changes requested by kbarrett (Reviewer).

test/hotspot/jtreg/gc/TestReferenceRefersTo.java line 166:

> 164:
> 165:     private static boolean isShenandoahIUMode() {
> 166:         return WB.getBooleanVMFlag("UseShenandoahGC") && "iu".equals(WB.getStringVMFlag("ShenandoahGCMode"));

This should be using sun.hotspot.gc.GC.Shenandoah.isSelected()

test/hotspot/jtreg/gc/TestReferenceRefersTo.java line 211:

> 209:             } else {
> 210:               expectNotCleared(testWeak4, "testWeak4");
> 211:             }

I think I would prefer to keep this test program "generic", rather than having this Shenandoah IU mode intrusion.  So remove the old check of testWeak4 state here, and remove the check of obj4 below.  Instead, change the later check of testWeak4 being notified, where the new test is that either testWeak4 and obj4 are both null (IU and the like) or both are non-null (SATB and others).  Then add a couple of tests programs for the specific clearing or not clearing expected behaviors, with appropriate `@requires` restrictions.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2653

[Kim Barrett-2]

On Sat, 20 Feb 2021 14:01:51 GMT, Kim Barrett <[hidden email]> wrote:

>> Shenandoah's IU mode allows referents to be cleared even when accessed during concurrent marking. The test TestReferenceRefersTo.java needs to be adjusted to allow for that.
>>
>> Test:
>>  - [x] TestReferenceRefersTo.java + Shenandoah/IU
>>  - [x] TestReferenceRefersTo.java + Shenandoah/SATB
>>  - [x] TestReferenceRefersTo.java + G1
>
> Changes requested by kbarrett (Reviewer).

Because this is a shared test, I suggest renaming the bug to something like "[TESTBUG] Fix TestReferenceRefersTo.java for Shenandoah IU mode", and remove the gc-shenandoah label.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2653

[Roman Kennke]

> Shenandoah's IU mode allows referents to be cleared even when accessed during concurrent marking. The test TestReferenceRefersTo.java needs to be adjusted to allow for that.
>
> Test:
>  - [x] TestReferenceRefersTo.java + Shenandoah/IU
>  - [x] TestReferenceRefersTo.java + Shenandoah/SATB
>  - [x] TestReferenceRefersTo.java + G1

Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:

  Split TestReferenceRefersTo test in generic and non-Shenandoah parts

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2653/files
  - new: https://git.openjdk.java.net/jdk/pull/2653/files/b1fffb02..9746bc85

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2653&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2653&range=00-01

  Stats: 230 lines in 2 files changed: 206 ins; 20 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2653.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2653/head:pull/2653

PR: https://git.openjdk.java.net/jdk/pull/2653

[Roman Kennke]

On Sat, 20 Feb 2021 13:55:33 GMT, Kim Barrett <[hidden email]> wrote:

> This should be using sun.hotspot.gc.GC.Shenandoah.isSelected()

Yes, but which int constant should be used there? Doesn't matter much, I'm not using this at all anymore, following your other suggestions.

> test/hotspot/jtreg/gc/TestReferenceRefersTo.java line 211:
>
>> 209:             } else {
>> 210:               expectNotCleared(testWeak4, "testWeak4");
>> 211:             }
>
> I think I would prefer to keep this test program "generic", rather than having this Shenandoah IU mode intrusion.  So remove the old check of testWeak4 state here, and remove the check of obj4 below.  Instead, change the later check of testWeak4 being notified, where the new test is that either testWeak4 and obj4 are both null (IU and the like) or both are non-null (SATB and others).  Then add a couple of tests programs for the specific clearing or not clearing expected behaviors, with appropriate `@requires` restrictions.

Right, that is even better. I made the base test generic, extracted the offending parts into its own test, and will push the Shenandoah specific test under a different PR. Thank you!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2653

[Roman Kennke]

> Shenandoah's IU mode allows referents to be cleared even when accessed during concurrent marking. The test TestReferenceRefersTo.java needs to be adjusted to allow for that.
>
> Test:
>  - [x] TestReferenceRefersTo.java + Shenandoah/IU
>  - [x] TestReferenceRefersTo.java + Shenandoah/SATB
>  - [x] TestReferenceRefersTo.java + G1

Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:

  Fix compilation failures after renames

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2653/files
  - new: https://git.openjdk.java.net/jdk/pull/2653/files/9746bc85..54a027c9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2653&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2653&range=01-02

  Stats: 8 lines in 1 file changed: 0 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2653.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2653/head:pull/2653

PR: https://git.openjdk.java.net/jdk/pull/2653

[Kim Barrett-2]

On Mon, 22 Feb 2021 15:12:03 GMT, Roman Kennke <[hidden email]> wrote:

>> Shenandoah's IU mode allows referents to be cleared even when accessed during concurrent marking. The test TestReferenceRefersTo.java needs to be adjusted to allow for that.
>>
>> Test:
>>  - [x] TestReferenceRefersTo.java + Shenandoah/IU
>>  - [x] TestReferenceRefersTo.java + Shenandoah/SATB
>>  - [x] TestReferenceRefersTo.java + G1
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix compilation failures after renames

Changes requested by kbarrett (Reviewer).

test/hotspot/jtreg/gc/TestReferenceRefersTo.java line 241:

> 239:             }
> 240:             if ((testWeak4 == null) != (obj4 == null)) {
> 241:                 fail("either referent is cleared and we got notified, or neither of this happened");

It might be helpful if the failure reported which one was non-null.  Not that it should ever fail...

test/hotspot/jtreg/gc/TestReferenceRefersToDuringConcMark.java line 96:

> 94:     }
> 95:
> 96:     private static void expectCleared(Reference<TestObject> ref,

Unused.

test/hotspot/jtreg/gc/TestReferenceRefersToDuringConcMark.java line 27:

> 25:
> 26: /* @test
> 27:  * @requires vm.gc != "Epsilon"

I think this test "works" for Epsilon just as well as it does for Serial or Parallel or any other GC that doesn't support concurrent breakpoints, and either all should be excluded or none.

test/hotspot/jtreg/gc/TestReferenceRefersToDuringConcMark.java line 28:

> 26: /* @test
> 27:  * @requires vm.gc != "Epsilon"
> 28:  * @requires vm.gc != "Shenandoah"

I think this test works for Shenandoah so long as it's not in IU mode.  Is that possible to exclude with another `@requires` constraint?

test/hotspot/jtreg/gc/TestReferenceRefersToDuringConcMark.java line 39:

> 37:  */
> 38:
> 39: import java.lang.ref.PhantomReference;

PhantomReference is unused.

test/hotspot/jtreg/gc/TestReferenceRefersToDuringConcMark.java line 145:

> 143:
> 144:         progress("acquire control of concurrent cycles");
> 145:         WB.concurrentGCAcquireControl();

I think this test could be made a lot smaller and more obvious if it was explicitly just testing the keep-alive behavior of Reference.get for most concurrent collectors, rather than being a trimmed down copy of the earlier test.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2653

[Roman Kennke]

> Shenandoah's IU mode allows referents to be cleared even when accessed during concurrent marking. The test TestReferenceRefersTo.java needs to be adjusted to allow for that.
>
> Test:
>  - [x] TestReferenceRefersTo.java + Shenandoah/IU
>  - [x] TestReferenceRefersTo.java + Shenandoah/SATB
>  - [x] TestReferenceRefersTo.java + G1

Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:

  Some more trimming

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2653/files
  - new: https://git.openjdk.java.net/jdk/pull/2653/files/54a027c9..51f2d695

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2653&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2653&range=02-03

  Stats: 90 lines in 2 files changed: 1 ins; 79 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2653.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2653/head:pull/2653

PR: https://git.openjdk.java.net/jdk/pull/2653

[Roman Kennke]

On Mon, 22 Feb 2021 15:25:46 GMT, Kim Barrett <[hidden email]> wrote:

>> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix compilation failures after renames
>
> test/hotspot/jtreg/gc/TestReferenceRefersToDuringConcMark.java line 27:
>
>> 25:
>> 26: /* @test
>> 27:  * @requires vm.gc != "Epsilon"
>
> I think this test "works" for Epsilon just as well as it does for Serial or Parallel or any other GC that doesn't support concurrent breakpoints, and either all should be excluded or none.

Right. I excluded none.

> test/hotspot/jtreg/gc/TestReferenceRefersToDuringConcMark.java line 28:
>
>> 26: /* @test
>> 27:  * @requires vm.gc != "Epsilon"
>> 28:  * @requires vm.gc != "Shenandoah"
>
> I think this test works for Shenandoah so long as it's not in IU mode.  Is that possible to exclude with another `@requires` constraint?

How would I do that? IU mode can only be distinguished by VM flag.

> test/hotspot/jtreg/gc/TestReferenceRefersToDuringConcMark.java line 145:
>
>> 143:
>> 144:         progress("acquire control of concurrent cycles");
>> 145:         WB.concurrentGCAcquireControl();
>
> I think this test could be made a lot smaller and more obvious if it was explicitly just testing the keep-alive behavior of Reference.get for most concurrent collectors, rather than being a trimmed down copy of the earlier test.

Right. I trimmed it some more. I think it's cleaner now.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2653

[Roman Kennke]

> Shenandoah's IU mode allows referents to be cleared even when accessed during concurrent marking. The test TestReferenceRefersTo.java needs to be adjusted to allow for that.
>
> Test:
>  - [x] TestReferenceRefersTo.java + Shenandoah/IU
>  - [x] TestReferenceRefersTo.java + Shenandoah/SATB
>  - [x] TestReferenceRefersTo.java + G1

Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:

  Fix another compilation failure

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2653/files
  - new: https://git.openjdk.java.net/jdk/pull/2653/files/51f2d695..44f99b86

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2653&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2653&range=03-04

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2653.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2653/head:pull/2653

PR: https://git.openjdk.java.net/jdk/pull/2653

[Kim Barrett-2]

On Mon, 22 Feb 2021 16:22:36 GMT, Roman Kennke <[hidden email]> wrote:

>> test/hotspot/jtreg/gc/TestReferenceRefersToDuringConcMark.java line 28:
>>
>>> 26: /* @test
>>> 27:  * @requires vm.gc != "Epsilon"
>>> 28:  * @requires vm.gc != "Shenandoah"
>>
>> I think this test works for Shenandoah so long as it's not in IU mode.  Is that possible to exclude with another `@requires` constraint?
>
> How would I do that? IU mode can only be distinguished by VM flag.

I've not tried it, but this might work.
`@requires vm.gc != "Shenandoah" | vm.opt.ShenandoahGCMode != "iu"`

-------------

PR: https://git.openjdk.java.net/jdk/pull/2653

[Roman Kennke]

> Shenandoah's IU mode allows referents to be cleared even when accessed during concurrent marking. The test TestReferenceRefersTo.java needs to be adjusted to allow for that.
>
> Test:
>  - [x] TestReferenceRefersTo.java + Shenandoah/IU
>  - [x] TestReferenceRefersTo.java + Shenandoah/SATB
>  - [x] TestReferenceRefersTo.java + G1

Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:

  Allow Shenandoah for TestReferenceRefersToDuringConcMark test, except IU mode

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2653/files
  - new: https://git.openjdk.java.net/jdk/pull/2653/files/44f99b86..8f4d8606

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2653&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2653&range=04-05

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2653.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2653/head:pull/2653

PR: https://git.openjdk.java.net/jdk/pull/2653

[Roman Kennke]

On Tue, 23 Feb 2021 08:24:51 GMT, Kim Barrett <[hidden email]> wrote:

>> How would I do that? IU mode can only be distinguished by VM flag.
>
> I've not tried it, but this might work.
> `@requires vm.gc != "Shenandoah" | vm.opt.ShenandoahGCMode != "iu"`

Indeed, it does. I changed the test requires accordingly.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2653

[Kim Barrett-2]

On Tue, 23 Feb 2021 08:44:58 GMT, Roman Kennke <[hidden email]> wrote:

>> Shenandoah's IU mode allows referents to be cleared even when accessed during concurrent marking. The test TestReferenceRefersTo.java needs to be adjusted to allow for that.
>>
>> Test:
>>  - [x] TestReferenceRefersTo.java + Shenandoah/IU
>>  - [x] TestReferenceRefersTo.java + Shenandoah/SATB
>>  - [x] TestReferenceRefersTo.java + G1
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
>
>   Allow Shenandoah for TestReferenceRefersToDuringConcMark test, except IU mode

Marked as reviewed by kbarrett (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2653

[Zhengyu Gu-3]

On Tue, 23 Feb 2021 08:44:58 GMT, Roman Kennke <[hidden email]> wrote:

>> Shenandoah's IU mode allows referents to be cleared even when accessed during concurrent marking. The test TestReferenceRefersTo.java needs to be adjusted to allow for that.
>>
>> Test:
>>  - [x] TestReferenceRefersTo.java + Shenandoah/IU
>>  - [x] TestReferenceRefersTo.java + Shenandoah/SATB
>>  - [x] TestReferenceRefersTo.java + G1
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
>
>   Allow Shenandoah for TestReferenceRefersToDuringConcMark test, except IU mode

Good to me.

-------------

Marked as reviewed by zgu (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2653

[Roman Kennke]

On Fri, 19 Feb 2021 19:48:51 GMT, Roman Kennke <[hidden email]> wrote:

> Shenandoah's IU mode allows referents to be cleared even when accessed during concurrent marking. The test TestReferenceRefersTo.java needs to be adjusted to allow for that.
>
> Test:
>  - [x] TestReferenceRefersTo.java + Shenandoah/IU
>  - [x] TestReferenceRefersTo.java + Shenandoah/SATB
>  - [x] TestReferenceRefersTo.java + G1

This pull request has now been integrated.

Changeset: c6eae061
Author:    Roman Kennke <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/c6eae061
Stats:     139 lines in 2 files changed: 127 ins; 7 del; 5 mod

8262049: [TESTBUG] Fix TestReferenceRefersTo.java for Shenandoah IU mode

Reviewed-by: kbarrett, zgu

-------------

PR: https://git.openjdk.java.net/jdk/pull/2653