-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8374878: Add Atomic<T>::compare_set #29135
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
Conversation
|
@kimbarrett, tell me what you think. I can polish and test it further if this is sensible. |
|
👋 Welcome back shade! A progress list of the required criteria for merging this PR into |
|
@shipilev 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: 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 8 new commits pushed to the
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 |
Webrevs
|
stefank
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but let's at least wait for Kim's review.
|
/reviewers 2 |
kimbarrett
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this addition. One optional suggestion.
|
First run of Linux x86_64 server fastdebug, |
kimbarrett
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Note that the recent updates invalidated prior approvals. So here's a re-approval.
|
|
||
| // Note that using "bottom" here might fail, since a pop_local might | ||
| // have decremented it. | ||
| assert_not_underflow(localBot, newAge.top()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[pre-existing] This assertion seems weird. It is checking newAge.top() after
already using been installed. Maybe it is misplaced? Or maybe it is checking
the wrong thing? Needs to be investigated. I've filed
https://bugs.openjdk.org/browse/JDK-8374915
|
/reviewers 1 |
|
Testing is still clean. I am integrating now, thanks. /integrate |
|
Going to push as commit 3368948.
Your commit was automatically rebased without conflicts. |
| return _value.compare_set(decay(compare_value), | ||
| decay(new_value), | ||
| order); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is subtly different from what we usually did with AtomicAccess / Atomic compare_exchange on translated types. We used to check equality with the bool operator==(const T&, constT&) rather than bool operator==(const PrimitiveConversions::Translate<T>::Decayed&, const PrimitiveConversions::Translate<T>::Decayed&).
For the PrimitiveConversions::Translate we currently have I do not think there is an issues as they are memcmp equivalent in both cases. But we may introduce a PrimitiveConversion in the future where this is not the case and this function returns false when using recover and bool operator==(const T&, constT&) would have returned true.
Following up on JDK-8367013 improvement, there is an opportunity to rewrite some of our low-level
cas(oldv, newv) == oldvpatterns to more straight-forward compare-and-set helper method. This is useful when you do not actually care about the result that used to be in memory or that is currently in memory, you only care if CAS succeeded or not.Java atomics already have this duality; arguably due to historical timeline of having compare-and-set before introducing compare-and-exchange.
Found this when converting Epsilon to Atomic (JDK-8374876), where this method would simplify the code a bit. I looked around current uses of
compare_exchangeand rewrote them tocompare_setto show what simplifications are possible.Additional testing:
allProgress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/29135/head:pull/29135$ git checkout pull/29135Update a local copy of the PR:
$ git checkout pull/29135$ git pull https://git.openjdk.org/jdk.git pull/29135/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 29135View PR using the GUI difftool:
$ git pr show -t 29135Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/29135.diff
Using Webrev
Link to Webrev Comment