Skip to content
This repository has been archived by the owner. It is now read-only.

8260684: vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java timed out #142

Closed
wants to merge 4 commits into from

Conversation

@kimbarrett
Copy link
Contributor

@kimbarrett kimbarrett commented Jun 24, 2021

Please review this fix of
vmTestbase/gc/gctests/PhantomReference/phantom001/phantom001.java to
eliminate races that can lead to the test occasionally timing out.

When the referent being tested by a particular iteration is a finalizable
class, the test invoked eatMemory() repeatedly until the referent's
finalize() function set a flag. Then, whether the referent is finalizable
or not, there is a call to eatMemory() to cause the phantom reference to be
cleared and notified. The testing thread then proceeds to wait for the
reference to be notified.

This has a problem with ZGC. One collection made the referent finalizable.
Reference processing and finalization will end up ensuring it will survive
the following collection. If the post-finalization eatMemory() is that next
collection, then it won't clear and notify the associated PhantomReference.
Usually that doesn't cause a problem because the referent will be reclaimed
by some other thread's later call to eatMemory(). But if the test is done
and terminating, there won't be a later GC, and the wait for notification
will block until the test times out.

There are other, rarer, scenarios with various collectors that could lead to
similar timeouts, but the one described above is "relatively" likely.

While investigating this issue I did a bunch of code cleanup (for example,
there was some really badly formatted code), refactoring, and commenting.
As a result, a large fraction of the test has changed, though mostly in ways
that don't affect it's function. The first of the two commits in the PR is
a whitespace-only cleanup. Excluding it may make reviewing easier.

Testing:
Ran the phantom002 test a couple thousand times.

Using a modified version of the test, verified that the described problem
case actually occurs, on the order of 1% of the time.


Progress

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

Issue

  • JDK-8260684: vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java timed out

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/142/head:pull/142
$ git checkout pull/142

Update a local copy of the PR:
$ git checkout pull/142
$ git pull https://git.openjdk.java.net/jdk17 pull/142/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 142

View PR using the GUI difftool:
$ git pr show -t 142

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/142.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 24, 2021

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

@openjdk openjdk bot added the rfr label Jun 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 24, 2021

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

  • hotspot-gc

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

@openjdk openjdk bot added the hotspot-gc label Jun 24, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 24, 2021

Webrevs

pliden
pliden approved these changes Jun 29, 2021
Copy link
Contributor

@pliden pliden left a comment

Looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 29, 2021

@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:

8260684: vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java timed out

Reviewed-by: pliden, lkorinth

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

@openjdk openjdk bot added the ready label Jun 29, 2021
Copy link
Contributor

@lkorinth lkorinth left a comment

Looks good to me!

private void eatMemory(int initialFactor) {
GarbageUtils.eatMemory(getExecutionController(),
garbageProducer,
initialFactor , 10, 0);
Copy link
Contributor

@lkorinth lkorinth Jun 30, 2021

remove space before comma

Copy link
Contributor Author

@kimbarrett kimbarrett Jul 1, 2021

Done.

@kimbarrett
Copy link
Contributor Author

@kimbarrett kimbarrett commented Jul 1, 2021

Thanks for reviews @pliden and @lkorinth

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jul 1, 2021

Going to push as commit 6c76e77.

@openjdk openjdk bot closed this Jul 1, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 1, 2021

@kimbarrett Pushed as commit 6c76e77.

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

@kimbarrett kimbarrett deleted the phantom_bug branch Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
3 participants