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

8268290: Improve LockFreeQueue<> utility #4379

Closed
wants to merge 4 commits into from

Conversation

kimbarrett
Copy link

@kimbarrett kimbarrett commented Jun 6, 2021

Please review this change to the LockFreeQueue utility class.

The LockFreeQueue originated as an implementation detail of
G1DirtyCardQueueSet, and was recently refactored into a public utility
class. In that refactoring it retained some limitations that were
acceptable in its original context, but may be problematic as a general
utility.

In particular, under some conditions a thread was not be able to pop the
last element in the queue, due to interference by a concurrent operation.
And this state will persist, so retrying the pop operation won't help until
the interfering thread had made sufficient progress. This was mitigated by
making the API more complex to provide notice to the client that the queue
may be in this state.

But it turns out we can do somewhat better, eliminating one of the
limitations, which is the point of this change. We introduce a
pseudo-object used as an end of queue marker. We can use the transition of
the last element's next value from the end marker to NULL by a pop operation
as a claim on the element, allowing the losing thread to recognize, retry,
and make progress.

This queue still has the limitation that an in-progress push/append may
prevent popping elements. Because of this, the class is being renamed to
NonblockingQueue. The old name suggests stronger guarantees than actually
provided.

The PR has two commits, the first for the functional changes, the second for
the renaming. The github diffs don't seem to be recognizing the renaming of
the source files as a rename, instead treating the old files as deleted and
the new files as added. The first commit by itself is probably more useful
for reviewing the functional changes.

Testing:
mach5 tier1-5


Progress

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

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4379

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 6, 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.

@kimbarrett kimbarrett marked this pull request as ready for review Jun 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 6, 2021

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

  • hotspot

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 hotspot rfr labels Jun 6, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 6, 2021

Webrevs

Copy link
Member

@walulyai walulyai left a comment

lgtm!

Copy link
Contributor

@tschatzl tschatzl left a comment

I think this is good.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 22, 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:

8268290: Improve LockFreeQueue<> utility

Reviewed-by: iwalulya, tschatzl

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 22, 2021
@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Jun 22, 2021

Thanks @walulyai and @tschatzl for reviews.

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Jun 22, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jun 22, 2021

Going to push as commit 0c693e2.

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

@openjdk openjdk bot commented Jun 22, 2021

@kimbarrett Pushed as commit 0c693e2.

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

@kimbarrett kimbarrett deleted the lfqueue branch Jun 22, 2021
@caoman
Copy link
Contributor

@caoman caoman commented Jun 25, 2021

Thank you for this fix. I like how append() can just store to &_head on CAS failure.

Minor comments:
Is it recommended to add SpinPause() in pop()'s while loop body?
This sentence above append() in inline.hpp could be removed: "it is an invariant that the old tail's "next" value is NULL".
I can make a PR for these separately.

@walulyai
Copy link
Member

@walulyai walulyai commented Jun 25, 2021

Minor comments:
Is it recommended to add SpinPause() in pop()'s while loop body?

I think with a try_pop method, if one needs to have a SpinPause, they can re-implement the retry-loop with the SpinPause and try_pop instead of having this in a generic pop().

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Jun 25, 2021

I don't think pop() needs SpinPause() in its body. It's not really an empty spin. It only loops on a cmpxchg failure. Someone might want to use try_pop() and SpinYield or something similar in a highly contended case, but I'd rather leave that to the specific case to parameterize.

You are correct that this comment should be removed: "it is an invariant that the old tail's "next" value is NULL". Oops, sorry I missed that.

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