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

8274516: [REDO] JDK-8271880: Tighten condition for excluding regions from collecting cards with cross-references #5786



Copy link

@tschatzl tschatzl commented Oct 1, 2021

Hi all,

can I have reviews for this redo of (JDK-8271880: Tighten condition for excluding regions from collecting cards with cross-references)[]?

The JDK-8271180 change has been backed out because it crashed fairly easily on (kitchensink)[] (and apparently also on some (ArrayJuggle tests)[]).

The reason is that the original change missed application of the barriers during removal of an element from the discovered list. This change fixes that, properly applying it (calling the enqueue methods).

This change consist of two commits at this time: first the revert of the backout for JDK-8271180, and a second one with the actual fixes. For people who already looked at the former I recommend only looking at that the latter.

There are three interesting changes:

  • change of the EnqueueDiscoveredFieldClosure::enqueue fields' first parameter to directly take the address to patch. This simplifies some code.
  • introduction of the barrier in the DiscoveredListIterator::remove method - this should be the only relevant place where the barrier application has been missing. There is some special handling when removing from the start of the discovered list - the address points into the C-heap at that time and we should not apply a barrier here.
  • the EnqueueDiscoveredFieldClosure::enqueue method filters out writes to non-live reference objects (as per the _is_alive closure) - we may get called during removal of elements, and the next element is also not live. There is no need to apply the barrier to those. Actually, leaving this out could trigger an assertion in G1ParScanThreadState::write_ref_field_post where it checks that the object we refer to that is in the collection set must be self-forwarded (i.e. an object where evacuation failed).

Testing: tier1-5, kitchensink (30min) 720 times with no crash, the failing ArrayJuggle28 test 2000 times



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


  • JDK-8274516: [REDO] JDK-8271880: Tighten condition for excluding regions from collecting cards with cross-references



Using git

Checkout this PR locally:
$ git fetch pull/5786/head:pull/5786
$ git checkout pull/5786

Update a local copy of the PR:
$ git checkout pull/5786
$ git pull pull/5786/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5786

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

Using diff file

Download this PR as a diff file:

…ng regions from collecting cards with cross-references"

This reverts commit 1dc8fa9.
Copy link

bridgekeeper bot commented Oct 1, 2021

👋 Welcome back tschatzl! 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.

Copy link

openjdk bot commented Oct 1, 2021

@tschatzl 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 Oct 1, 2021
@tschatzl tschatzl marked this pull request as ready for review October 1, 2021 12:04
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 1, 2021
Copy link

mlbridge bot commented Oct 1, 2021


Copy link

@kstefanj kstefanj left a comment

Choose a reason for hiding this comment

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

Looks good. Just some small comments.

src/hotspot/share/gc/shared/referenceProcessor.cpp Outdated Show resolved Hide resolved
Copy link

openjdk bot commented Oct 5, 2021

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

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

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

8274516: [REDO] JDK-8271880: Tighten condition for excluding regions from collecting cards with cross-references

Reviewed-by: sjohanss, ayang

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 125 new commits pushed to the master branch:

  • e16b93a: 8274770: [PPC64] resolve_jobject needs a generic implementation to support load barriers
  • 1ab6414: 8275051: Shenandoah: Correct ordering of requested gc cause and gc request flag
  • b460d6d: 8275091: /src/ has non-canonical order
  • d04d4ee: 8274894: Use Optional.empty() instead of ofNullable(null) in HttpResponse.BodySubscribers.discarding
  • 33050f8: 8274986: max code printed in hs-err logs should be configurable
  • 8de2636: 8274615: Support relaxed atomic add for linux-aarch64
  • 7d2633f: 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE
  • cfe7471: 8177814: jdk/editpad is not in jdk TEST.groups
  • a5f09d1: 8275031: runtime/ErrorHandling/ fails when hsdis is present
  • ef0922e: 8274560: JFR: Add test for OldObjectSample event when using Shenandoah
  • ... and 115 more:

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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 5, 2021
Copy link
Contributor Author

Thanks @albertnetymk @kstefanj for your reviews.


Copy link

openjdk bot commented Oct 13, 2021

Going to push as commit c3b75c6.
Since your change was applied there have been 137 commits pushed to the master branch:

  • 337b73a: 8274851: [PPC64] Port zgc to linux on ppc64le
  • cf82867: 8275049: [ZGC] missing null check in ZNMethod::log_register
  • ab34cce: 8275186: Suppress warnings on non-serializable array component types in xml
  • b1b8350: 8275171: ProblemList compiler/codegen/aes/ on linux-x64 and windows-x64 in -Xcomp mode
  • 03c2b73: 8275128: Build hsdis using normal build system
  • 124f823: 8268764: Use Long.hashCode() instead of int-cast where applicable
  • 8657f77: 8271514: support JFR use of new ThreadsList::Iterator
  • b8bd259: 8271737: Only normalize the cached user.dir property once
  • 89999f7: 8275131: Exceptions after a touchpad gesture on macOS
  • 07b1f1c: 8274548: (fc) FileChannel gathering write fails with IOException "Invalid argument" on macOS 11.6
  • ... and 127 more:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 13, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 13, 2021
Copy link

openjdk bot commented Oct 13, 2021

@tschatzl Pushed as commit c3b75c6.

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

@tschatzl tschatzl deleted the submit/8274516-redo-8271880-tighten branch October 13, 2021 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
hotspot-gc integrated Pull request has been integrated
3 participants