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

v1.9.0 #98

Merged
merged 1 commit into from
Feb 22, 2024
Merged

v1.9.0 #98

merged 1 commit into from
Feb 22, 2024

Conversation

notgull
Copy link
Member

@notgull notgull commented Feb 17, 2024

@taiki-e
Copy link
Collaborator

taiki-e commented Feb 18, 2024

  • Use weaker atomic orderings for notifications.

I haven't seen the relevant code yet, but I think notifications are usually preferable to having SeqCst semantics, does this change that semantics?
https://docs.rs/event-listener/latest/event_listener/trait.IntoNotification.html#method.relaxed

Usually, notifications emit a SeqCst atomic fence before any listeners are woken up. This ensures that notification state isn’t inconsistent before any wakers are woken up.

@notgull
Copy link
Member Author

notgull commented Feb 18, 2024

I haven't seen the relevant code yet, but I think notifications are usually preferable to having SeqCst semantics, does this change that semantics? https://docs.rs/event-listener/latest/event_listener/trait.IntoNotification.html#method.relaxed

The actual notification data structure (the Sleepers structure in this instance) is secured by a Mutex. So by my count the current SeqCst ordering is redundant, as:

  • The Release ordering of releasing the Mutex guard should ensure that the store to notified is kept ordered when it is stored.
  • The AcqRel ordering of swapping out the notified atomic shouldn't be problematic as it is synchronized with the Release ordering of setting the value to true.

In addition, AtomicWaker uses similar orderings, so I think it should be fine.

cc @james7132, this is your change, you might have a better mental model of what's happening here.

@james7132
Copy link
Contributor

james7132 commented Feb 18, 2024

The way that I saw it was that it was more acting as an early-out in the case there are multiple notifications going off while a notified thread isn't fully awake yet. In that case, the Acquire and Release orderings are sufficient to act as a lock, in the exact way that @notgull covered. If that's the wrong mental model for how that should be working, then it should be reverted.

@notgull notgull requested review from taiki-e and fogti February 18, 2024 20:09
Copy link
Collaborator

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification!

@notgull
Copy link
Member Author

notgull commented Feb 22, 2024

Given the new changes on master I've elected to make this a minor version bump.

@taiki-e taiki-e changed the title v1.8.1 v1.9.0 Feb 22, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull merged commit 2f3189a into master Feb 22, 2024
8 checks passed
@notgull notgull deleted the notgull/next branch February 22, 2024 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants