search

Amanieu / thread_local-rs

Per-object thread-local storage for Rust
Apache License 2.0
328 stars 41 forks source link

Replace non-atomic load with an atomic one #72

Closed james7132 closed 7 months ago

james7132 commented 7 months ago

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

james7132 commented 7 months ago

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 commented 7 months ago

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.

james7132 commented 7 months ago

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 commented 7 months ago

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.