Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v3]
On Tue, 8 Dec 2020 17:30:11 GMT, Mandy Chung wrote: >> Kim Barrett has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - Merge branch 'master' into no_isenqueued >> - move REMOVE and RETAIN decls and init >> - update WeakReferenceGC test >> - update ReferenceQueue test >> - update ReferencesGC test > > Marked as reviewed by mchung (Reviewer). Thanks for reviews @mlchung and @tschatzl - PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v3]
> Please review this change that eliminates the use of Reference.isEnqueued by > tests. There were three tests using it: > > vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java > vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java > jdk/java/lang/ref/ReferenceEnqueue.java > > In each of them, some combination of using Reference.refersTo and > ReferenceQueue.remove with a timeout were used to eliminate the use of > Reference.isEnqueued. > > I also cleaned up ReferencesGC.java in various respects. It contained > several bits of dead code, and the failure checks were made stronger. > > Testing: > mach5 tier1 > Locally (linux-x64) ran all three tests with each GC (including Shenandoah). Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge branch 'master' into no_isenqueued - move REMOVE and RETAIN decls and init - update WeakReferenceGC test - update ReferenceQueue test - update ReferencesGC test - Changes: - all: https://git.openjdk.java.net/jdk/pull/1691/files - new: https://git.openjdk.java.net/jdk/pull/1691/files/01710567..d5355342 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1691=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1691=01-02 Stats: 7952 lines in 328 files changed: 5091 ins; 1646 del; 1215 mod Patch: https://git.openjdk.java.net/jdk/pull/1691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1691/head:pull/1691 PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v2]
On Thu, 10 Dec 2020 08:56:25 GMT, Kim Barrett wrote: >> I understand that the test code previously just forwarded the >> `InterruptedException` if it happened in the `Thread.sleep()` call too. So >> this may only be an exiting issue and please ignore this comment. >> Not catching `InterruptedException` here only seems to be a cause for >> unnecessary failure. Then again, it probably does not happen a lot. > > Nothing in the test calls Thread.interrupt(), so there isn't a risk of > failure due to not handling that exception in some "interesting" way. But > InterruptedException must be "handled" somehow, because it's a checked > exception. That's already dealt with by the run() method declaring that it > throws that type, and main declaring that it throws Exception. The other > tests modified in this change don't take that approach (just let it > propagate out through main), instead wrapping the interruptable calls in > try/catch, though again just to satisfy the requirement that a checked > exception must be statically verified to be handled, even though there > aren't going to be any thrown. Okay. - PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v2]
On Thu, 10 Dec 2020 09:01:54 GMT, Kim Barrett wrote: >> Please review this change that eliminates the use of Reference.isEnqueued by >> tests. There were three tests using it: >> >> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java >> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java >> jdk/java/lang/ref/ReferenceEnqueue.java >> >> In each of them, some combination of using Reference.refersTo and >> ReferenceQueue.remove with a timeout were used to eliminate the use of >> Reference.isEnqueued. >> >> I also cleaned up ReferencesGC.java in various respects. It contained >> several bits of dead code, and the failure checks were made stronger. >> >> Testing: >> mach5 tier1 >> Locally (linux-x64) ran all three tests with each GC (including Shenandoah). > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > move REMOVE and RETAIN decls and init Also good with deferring the changes to the comments and the move of the statics. - Marked as reviewed by tschatzl (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v2]
> Please review this change that eliminates the use of Reference.isEnqueued by > tests. There were three tests using it: > > vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java > vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java > jdk/java/lang/ref/ReferenceEnqueue.java > > In each of them, some combination of using Reference.refersTo and > ReferenceQueue.remove with a timeout were used to eliminate the use of > Reference.isEnqueued. > > I also cleaned up ReferencesGC.java in various respects. It contained > several bits of dead code, and the failure checks were made stronger. > > Testing: > mach5 tier1 > Locally (linux-x64) ran all three tests with each GC (including Shenandoah). Kim Barrett has updated the pull request incrementally with one additional commit since the last revision: move REMOVE and RETAIN decls and init - Changes: - all: https://git.openjdk.java.net/jdk/pull/1691/files - new: https://git.openjdk.java.net/jdk/pull/1691/files/e87206a8..01710567 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1691=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1691=00-01 Stats: 6 lines in 1 file changed: 4 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1691/head:pull/1691 PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v2]
On Wed, 9 Dec 2020 13:59:09 GMT, Thomas Schatzl wrote:
>> test/jdk/java/lang/ref/ReferenceEnqueue.java line 58:
>>
>>> 56: for (int i = 0; i < iterations; i++) {
>>> 57: System.gc();
>>> 58: enqueued = (queue.remove(100) == ref);
>>
>> The code does not catch `InterruptedException` like it does in the other
>> files.
>
> I understand that the test code previously just forwarded the
> `InterruptedException` if it happened in the `Thread.sleep()` call too. So
> this may only be an exiting issue and please ignore this comment.
> Not catching `InterruptedException` here only seems to be a cause for
> unnecessary failure. Then again, it probably does not happen a lot.
Nothing in the test calls Thread.interrupt(), so there isn't a risk of
failure due to not handling that exception in some "interesting" way. But
InterruptedException must be "handled" somehow, because it's a checked
exception. That's already dealt with by the run() method declaring that it
throws that type, and main declaring that it throws Exception. The other
tests modified in this change don't take that approach (just let it
propagate out through main), instead wrapping the interruptable calls in
try/catch, though again just to satisfy the requirement that a checked
exception must be statically verified to be handled, even though there
aren't going to be any thrown.
-
PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests
On Wed, 9 Dec 2020 13:26:04 GMT, Thomas Schatzl wrote: >> Please review this change that eliminates the use of Reference.isEnqueued by >> tests. There were three tests using it: >> >> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java >> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java >> jdk/java/lang/ref/ReferenceEnqueue.java >> >> In each of them, some combination of using Reference.refersTo and >> ReferenceQueue.remove with a timeout were used to eliminate the use of >> Reference.isEnqueued. >> >> I also cleaned up ReferencesGC.java in various respects. It contained >> several bits of dead code, and the failure checks were made stronger. >> >> Testing: >> mach5 tier1 >> Locally (linux-x64) ran all three tests with each GC (including Shenandoah). > > test/hotspot/jtreg/vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java line > 129: > >> 127: } >> 128: >> 129: int REMOVE = (int) (RANGE * RATIO); > > These two constants could be factored out as static finals to match the > casing. I'm making REMOVE and RETAIN statics, near RANGE and RATIO. (Meant to do that before, but forgot.) They can't be final though, because RANGE and RATIO aren't final, and can be set from command line arguments. So they'll get initialized in parseArgs. - PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests
On Wed, 9 Dec 2020 13:28:44 GMT, Thomas Schatzl wrote: >> Please review this change that eliminates the use of Reference.isEnqueued by >> tests. There were three tests using it: >> >> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java >> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java >> jdk/java/lang/ref/ReferenceEnqueue.java >> >> In each of them, some combination of using Reference.refersTo and >> ReferenceQueue.remove with a timeout were used to eliminate the use of >> Reference.isEnqueued. >> >> I also cleaned up ReferencesGC.java in various respects. It contained >> several bits of dead code, and the failure checks were made stronger. >> >> Testing: >> mach5 tier1 >> Locally (linux-x64) ran all three tests with each GC (including Shenandoah). > > Changes requested by tschatzl (Reviewer). > [pre-existing] The topWeakReferenceGC.java description at the top describes > that the test calls System.gc() explicitly to trigger garbage collections at > the end. It does not. Maybe this could be weasel-worded around like in the > other cases in that text. There are a lot of things much more wrong with that comment. Doing more GCs doesn't cause more enqueues to happen. The "non-deterministic" enqueuing is just a race. The GC adds references to the pending list. The reference processing thread transfers references from the pending list to their associated queue (if any). The test code is racing with that. The change to use Reference.remove with a timeout eliminates all that, and one GC should be. Addressing all that would be a substantial rewrite of this test though. Mind if I defer that to a new RFE? - PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests
On Wed, 9 Dec 2020 13:23:47 GMT, Thomas Schatzl wrote:
>> Please review this change that eliminates the use of Reference.isEnqueued by
>> tests. There were three tests using it:
>>
>> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java
>> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java
>> jdk/java/lang/ref/ReferenceEnqueue.java
>>
>> In each of them, some combination of using Reference.refersTo and
>> ReferenceQueue.remove with a timeout were used to eliminate the use of
>> Reference.isEnqueued.
>>
>> I also cleaned up ReferencesGC.java in various respects. It contained
>> several bits of dead code, and the failure checks were made stronger.
>>
>> Testing:
>> mach5 tier1
>> Locally (linux-x64) ran all three tests with each GC (including Shenandoah).
>
> test/jdk/java/lang/ref/ReferenceEnqueue.java line 58:
>
>> 56: for (int i = 0; i < iterations; i++) {
>> 57: System.gc();
>> 58: enqueued = (queue.remove(100) == ref);
>
> The code does not catch `InterruptedException` like it does in the other
> files.
I understand that the test code previously just forwarded the
`InterruptedException` if it happened in the `Thread.sleep()` call too. So this
may only be an exiting issue and please ignore this comment.
Not catching `InterruptedException` here only seems to be a cause for
unnecessary failure. Then again, it probably does not happen a lot.
-
PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests
On Tue, 8 Dec 2020 09:52:51 GMT, Kim Barrett wrote:
> Please review this change that eliminates the use of Reference.isEnqueued by
> tests. There were three tests using it:
>
> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java
> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java
> jdk/java/lang/ref/ReferenceEnqueue.java
>
> In each of them, some combination of using Reference.refersTo and
> ReferenceQueue.remove with a timeout were used to eliminate the use of
> Reference.isEnqueued.
>
> I also cleaned up ReferencesGC.java in various respects. It contained
> several bits of dead code, and the failure checks were made stronger.
>
> Testing:
> mach5 tier1
> Locally (linux-x64) ran all three tests with each GC (including Shenandoah).
Changes requested by tschatzl (Reviewer).
test/jdk/java/lang/ref/ReferenceEnqueue.java line 58:
> 56: for (int i = 0; i < iterations; i++) {
> 57: System.gc();
> 58: enqueued = (queue.remove(100) == ref);
The code does not catch `InterruptedException` like it does in the other files.
test/hotspot/jtreg/vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java line
129:
> 127: }
> 128:
> 129: int REMOVE = (int) (RANGE * RATIO);
These two constants could be factored out as static finals to match the casing.
-
PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests
On Tue, 8 Dec 2020 17:30:11 GMT, Mandy Chung wrote: >> Please review this change that eliminates the use of Reference.isEnqueued by >> tests. There were three tests using it: >> >> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java >> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java >> jdk/java/lang/ref/ReferenceEnqueue.java >> >> In each of them, some combination of using Reference.refersTo and >> ReferenceQueue.remove with a timeout were used to eliminate the use of >> Reference.isEnqueued. >> >> I also cleaned up ReferencesGC.java in various respects. It contained >> several bits of dead code, and the failure checks were made stronger. >> >> Testing: >> mach5 tier1 >> Locally (linux-x64) ran all three tests with each GC (including Shenandoah). > > Marked as reviewed by mchung (Reviewer). I'm not able to put this in the appropriate place using the github UI: [pre-existing] The topWeakReferenceGC.java description at the top describes that the test calls System.gc() explicitly to trigger garbage collections at the end. It does not. Maybe this could be weasel-worded around like in the other cases in that text. - PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests
On Tue, 8 Dec 2020 09:52:51 GMT, Kim Barrett wrote: > Please review this change that eliminates the use of Reference.isEnqueued by > tests. There were three tests using it: > > vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java > vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java > jdk/java/lang/ref/ReferenceEnqueue.java > > In each of them, some combination of using Reference.refersTo and > ReferenceQueue.remove with a timeout were used to eliminate the use of > Reference.isEnqueued. > > I also cleaned up ReferencesGC.java in various respects. It contained > several bits of dead code, and the failure checks were made stronger. > > Testing: > mach5 tier1 > Locally (linux-x64) ran all three tests with each GC (including Shenandoah). Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1691
