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

8269870: PS: Membar in PSPromotionManager::copy_unmarked_to_survivor_space could be relaxed #4751

Closed
wants to merge 2 commits into from

Conversation

Hamlin-Li
Copy link

@Hamlin-Li Hamlin-Li commented Jul 12, 2021

Membar in PSPromotionManager::copy_unmarked_to_survivor_space could be relaxed.
( Please also refer to the similar membar relaxed in G1ParScanThreadState::do_copy_to_survivor_space )

Per the specjbb2015 test results, it has about 8% improvement in critical.
https://bugs.openjdk.java.net/secure/attachment/95445/jdk17_default_options.v2.png


Progress

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

Issue

  • JDK-8269870: PS: Membar in PSPromotionManager::copy_unmarked_to_survivor_space could be relaxed

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4751

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 12, 2021

👋 Welcome back mli! 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 Jul 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 12, 2021

@Hamlin-Li 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 Jul 12, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 12, 2021

Webrevs

@kimbarrett
Copy link

@kimbarrett kimbarrett commented Jul 12, 2021

A change of this kind requires careful analysis and supporting arguments that it is correct. As mentioned in the bug comments, this same change was proposed and extensively discussed previously (JDK-8154736), and rejected. What has changed that makes this even worth discussing again? It is the responsibility of the author to make those arguments and provide the needed evidence.

Simply referring to a similar part of G1 is not sufficient evidence for the correctness of this change. G1 and Parallel have significant differences; it cannot be assumed that something that works for one will work for the other.

@Hamlin-Li
Copy link
Author

@Hamlin-Li Hamlin-Li commented Jul 12, 2021

I thought it's because of some log message in PS which needs to refer to forwardee's content, and the log messages is now gone.
But let me do a more comprehensive investigation, will update later.

@kimbarrett
Copy link

@kimbarrett kimbarrett commented Jul 12, 2021

I thought it's because of some log message in PS which needs to refer to forwardee's content, and the log messages is now gone.

Those were the immediately obvious reasons why it didn't work. But there was also discussion about the problems that could arise because the forwardee was being returned to various places that might also have problems. And even if they didn't at the time, there was concern over the fragility that would result, as those places using the "copied" object would need to always be careful about touching the contents of the object.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 9, 2021

@Hamlin-Li This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 6, 2021

@Hamlin-Li This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc rfr
2 participants