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

8269596: Snapshot soft ref policy before marking/copying #4624

Closed
wants to merge 3 commits into from

Conversation

albertnetymk
Copy link
Member

@albertnetymk albertnetymk commented Jun 29, 2021

Set up soft ref policy before marking/copying for Serial, Parallel and G1. With this change, all collectors do soft-ref policy setup in the beginning of a GC cycle.


Progress

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

Issue

  • JDK-8269596: Snapshot soft ref policy before marking/copying

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4624

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 29, 2021

👋 Welcome back ayang! 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 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 29, 2021

@albertnetymk 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 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 29, 2021

Webrevs

Copy link
Contributor

@tschatzl tschatzl left a comment

Would it be useful to only call the soft ref policy setup via setup_policy() when it is (potentially) set to true? I.e. only during full gc, and call setup_policy(false) afterwards, maybe with a scope object?
This would remove the unnecessary calls with false as argument completely in the places where we never actually change it (e.g. for the G1 concurrent mark ref processor), and emphasize for the stw full gc collectors that they are the only ones eventually change it to "always clearing". Feel free to ignore this suggestion, but it seems unnecessary to have setup_policy(false); calls in a lot of places. Maybe if combined with below suggestion this one is obsolete.

Also the sequence

reference_processor()->enable_discovery();
reference_processor()->setup_policy(<some value>);

should be merged into a single call as there are like five places now where both are called one after another.

In psParallelCompact.cpp, the marked_for_unloading local right after these calls could be removed too, it's unused.

@albertnetymk
Copy link
Member Author

@albertnetymk albertnetymk commented Jul 1, 2021

Merged enable_discovery and setup_policy into one call, and made setup_policy private.

Copy link
Contributor

@tschatzl tschatzl left a comment

Lgtm.

@openjdk
Copy link

@openjdk openjdk bot commented Jul 1, 2021

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

8269596: Snapshot soft ref policy before marking/copying

Reviewed-by: tschatzl, kbarrett

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

  • 4107dcf: 8269466: Factor out the common code for initializing and starting internal VM JavaThreads
  • 2baf498: 8269743: test/hotspot/jtreg/vmTestbase/vm/mlvm/meth/stress/jni/nativeAndMH/Test.java crash with small heap (-Xmx50m)
  • 589f084: 8269110: ZGC: Remove dead code in zBarrier
  • b0e1867: Merge
  • a4d2a9a: 8269745: [JVMCI] restore original qualified exports to Graal
  • e377397: 8268566: java/foreign/TestResourceScope.java timed out
  • 6c76e77: 8260684: vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java timed out
  • 4bbf11d: 8269580: assert(is_valid()) failed: invalid register (-1)
  • 54dd510: 8269704: Typo in j.t.Normalizer.normalize()
  • a8385fe: 8269354: javac crashes when processing parenthesized pattern in instanceof
  • ... and 66 more: https://git.openjdk.java.net/jdk/compare/d0d26f5c550e44bf22d9155e4219351104be4635...master

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 label Jul 1, 2021
@albertnetymk
Copy link
Member Author

@albertnetymk albertnetymk commented Jul 1, 2021

I accidentally removed a call to disable_discovery(); in genCollectedHeap.cpp. Restored in the latest commit.

Copy link

@kimbarrett kimbarrett left a comment

Looks good.

@albertnetymk
Copy link
Member Author

@albertnetymk albertnetymk commented Jul 2, 2021

Thanks for the review.

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jul 2, 2021

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

  • 4107dcf: 8269466: Factor out the common code for initializing and starting internal VM JavaThreads
  • 2baf498: 8269743: test/hotspot/jtreg/vmTestbase/vm/mlvm/meth/stress/jni/nativeAndMH/Test.java crash with small heap (-Xmx50m)
  • 589f084: 8269110: ZGC: Remove dead code in zBarrier
  • b0e1867: Merge
  • a4d2a9a: 8269745: [JVMCI] restore original qualified exports to Graal
  • e377397: 8268566: java/foreign/TestResourceScope.java timed out
  • 6c76e77: 8260684: vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java timed out
  • 4bbf11d: 8269580: assert(is_valid()) failed: invalid register (-1)
  • 54dd510: 8269704: Typo in j.t.Normalizer.normalize()
  • a8385fe: 8269354: javac crashes when processing parenthesized pattern in instanceof
  • ... and 66 more: https://git.openjdk.java.net/jdk/compare/d0d26f5c550e44bf22d9155e4219351104be4635...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 2, 2021
@albertnetymk albertnetymk deleted the soft-setup branch Jul 2, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 2, 2021

@albertnetymk Pushed as commit f8bcbf0.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc integrated
3 participants