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

Android/Linux/rdrand: Use once_cell::race::OnceBool instead of LazyBool. #483

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

briansmith
Copy link
Contributor

@briansmith briansmith commented Jun 18, 2024

Remove src/lazy.rs.

lazy::LazyBool had "last to win the race" semantics. When multiple threads see an uninitialized LazyBool, all of them will calculate a value. As they finish, each one will overwrite the value set by the thread that finished previously. If two threads calculate different values for the boolean, then the value of the boolean can change during the period where the threads are racing. This doesn't seem to be a huge issue with the way it is currently used, but it is hard to reason about.

once_cell::race::OnceBool has "first to win the race" semantics. When multiple threads see an uninitialized OnceBool, all of them will calculate a value. The first one to finish will write its value; the rest will have their work ignored. Thus there is never any change in the stored value at any point. This is much easier to reason about.

The different semantics come down to the fact that once_cell uses AtomicUsize::compare_exchange whereas lazy.rs was using AtomicUsize::store.

@newpavlov
Copy link
Member

I don't think that adding a new dependency to save less than 10 lines of straightforward code is worthwhile.

@briansmith
Copy link
Contributor Author

I don't think that adding a new dependency to save less than 10 lines of straightforward code is worthwhile.

This is designed to accompany #481, which adds a more important once_cell dependency.

@briansmith briansmith force-pushed the b/oncecell-lazy branch 2 times, most recently from cdd0a1b to 4f59687 Compare June 18, 2024 22:34
@briansmith briansmith force-pushed the b/oncecell-lazy branch 3 times, most recently from de49003 to 3370fb9 Compare June 19, 2024 05:46
@briansmith briansmith changed the title Use once_cell::race::OnceBool instead of defining our own LazyBool. Android/Linux/rdrand: Use once_cell::race::OnceBool instead of LazyBool. Jun 19, 2024
@briansmith briansmith marked this pull request as ready for review June 19, 2024 05:50
@briansmith
Copy link
Contributor Author

I updated the commit message (and the GitHub issue description) to emphasize the difference in semantics, before and after.

Remove src/lazy.rs.

`lazy::LazyBool` had "last to win the race" semantics. When multiple
threads see an uninitialized `LazyBool`, all of them will calculate a
value. As they finish, each one will overwrite the value set by the
thread that finished previously. If two threads calculate different
values for the boolean, then the value of the boolean can change
during the period where the threads are racing. This doesn't seem to be
a huge issue with the way it is currently used, but it is hard to
reason about.

`once_cell::race::OnceBool` has "first to win the race" semantics. When
multiple threads see an uninitialized `OnceBool`, all of them will
calculate a value. The first one to finish will write its value; the
rest will have their work ignored. Thus there is never any change in
the stored value at any point. This is much easier to reason about.

The different semantics come down to the fact that once_cell uses
`AtomicUsize::compare_exchange` whereas lazy.rs was using
`AtomicUsize::store`.
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