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

Executor: Replace unnecessary atomics in runqueue #1361

Merged
merged 1 commit into from
Apr 15, 2023

Conversation

GrantM11235
Copy link
Contributor

Only the head pointer needs to be atomic. The RunQueueItem pointers are only loaded and stored, and never concurrently

@Dirbaio
Copy link
Member

Dirbaio commented Apr 14, 2023

This could use the core (not atomic-polyfill) AtomicPtr instead. It has no overhead for loads/stores, and there's less unsafe.

The only downside is it's not available for riscv, but that should get solved soon.

@GrantM11235
Copy link
Contributor Author

I don't think that using AtomicPtr is actually any safer in this case. Any bug that could cause a data race here would almost certainly also lead to UB somewhere else. I think that using an atomic here actually makes it harder to reason about, because it falsely implies that there can/will be concurrent accesses.

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

Fair! SGTM1 Also using SyncUnsafeCell allows using Option which is nice.

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 15, 2023

Build succeeded:

@bors bors bot merged commit 1fdce6e into embassy-rs:master Apr 15, 2023
@GrantM11235 GrantM11235 deleted the runqueue-remove-atomics branch April 15, 2023 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants