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

Replace non-atomic load with an atomic one #72

Merged
merged 6 commits into from
Feb 20, 2024
Merged

Replace non-atomic load with an atomic one #72

merged 6 commits into from
Feb 20, 2024

Conversation

james7132
Copy link
Contributor

Fixes #70 by replacing the non-atomic load in get_inner with an atomic one.

Copy link

@notgull notgull left a comment

Choose a reason for hiding this comment

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

You should add a test that makes sure the issue isn't still active.

@james7132
Copy link
Contributor Author

You should add a test that makes sure the issue isn't still active.

Tried earlier, and it's surprisingly hard to get miri to fire with a minimal repro. I'll take another stab at this soon.

@james7132
Copy link
Contributor Author

james7132 commented Feb 18, 2024

Added a test, made sure it fails with miri on master, and then added miri to the CI to ensure that the test (and others) are tested against miri for all future PRs.

src/lib.rs Outdated Show resolved Hide resolved
@james7132
Copy link
Contributor Author

As a final sanity check, I reran the benchmarks in the repo, and it seems like this shouldn't negatively affect performance, at least not on x86 platforms:

get                     time:   [2.6138 ns 2.6172 ns 2.6216 ns]
                        change: [-2.8757% -2.6826% -2.4831%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
  6 (6.00%) high mild
  4 (4.00%) high severe

insert                  time:   [9.9271 ns 10.080 ns 10.221 ns]
                        change: [-6.3852% -3.7294% -1.0880%] (p = 0.01 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  4 (4.00%) high m

@Amanieu
Copy link
Owner

Amanieu commented Feb 20, 2024

We are waiting on #69 to fix CI. This will bump the Rust version used in CI, so you will be able to use black_box for benchmarks.

@Amanieu Amanieu merged commit 7db3f74 into Amanieu:master Feb 20, 2024
2 checks passed
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.

Miri reported undefined behavior when using ThreadLocal::iter concurrently from multiple threads.
3 participants