Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8257876: Avoid Reference.isEnqueued in tests #1691

Closed
wants to merge 5 commits into from

Conversation

@kimbarrett
Copy link

@kimbarrett kimbarrett commented Dec 8, 2020

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).

/label core-libs, hotspot-gc


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1691/head:pull/1691
$ git checkout pull/1691

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 8, 2020

👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 8, 2020

@kimbarrett Unknown command labels - for a list of valid commands use /help.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 8, 2020

@kimbarrett The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot-gc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

Loading

@kimbarrett kimbarrett marked this pull request as ready for review Dec 8, 2020
@openjdk openjdk bot added the rfr label Dec 8, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 8, 2020

Webrevs

Loading

mlchung
mlchung approved these changes Dec 8, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 8, 2020

@kimbarrett This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8257876: Avoid Reference.isEnqueued in tests

Reviewed-by: mchung, tschatzl

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

  • 4a839e9: 8256459: java/net/httpclient/ManyRequests.java and java/net/httpclient/LineBodyHandlerTest.java fail infrequently with java.net.ConnectException: Connection timed out: no further information

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Dec 8, 2020
@tschatzl
Copy link
Contributor

@tschatzl tschatzl commented Dec 9, 2020

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.

Loading

}

Thread.sleep(100);
enqueued = (queue.remove(100) == ref);
Copy link
Contributor

@tschatzl tschatzl Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code does not catch InterruptedException like it does in the other files.

Loading

Copy link
Contributor

@tschatzl tschatzl Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Author

@kimbarrett kimbarrett Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Contributor

@tschatzl tschatzl Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

Loading

}

for (int i = 0; i < (int) (RANGE * RATIO); i++) {
int REMOVE = (int) (RANGE * RATIO);
Copy link
Contributor

@tschatzl tschatzl Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two constants could be factored out as static finals to match the casing.

Loading

Copy link
Author

@kimbarrett kimbarrett Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Dec 10, 2020

[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?

Loading

Copy link
Contributor

@tschatzl tschatzl left a comment

Also good with deferring the changes to the comments and the move of the statics.

Loading

}

Thread.sleep(100);
enqueued = (queue.remove(100) == ref);
Copy link
Contributor

@tschatzl tschatzl Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

Loading

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Dec 10, 2020

/integrate

Loading

@openjdk openjdk bot closed this Dec 10, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 10, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 10, 2020

@kimbarrett Since your change was applied there has been 1 commit pushed to the master branch:

  • 4a839e9: 8256459: java/net/httpclient/ManyRequests.java and java/net/httpclient/LineBodyHandlerTest.java fail infrequently with java.net.ConnectException: Connection timed out: no further information

Your commit was automatically rebased without conflicts.

Pushed as commit db5da96.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Dec 10, 2020

Thanks for reviews @mlchung and @tschatzl

Loading

@kimbarrett kimbarrett deleted the no_isenqueued branch Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants