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

Use atomic tag counter #118

Merged
merged 2 commits into from
Aug 31, 2022
Merged

Use atomic tag counter #118

merged 2 commits into from
Aug 31, 2022

Conversation

giordano
Copy link
Member

@giordano giordano commented Mar 28, 2022

I need think more about this, but this doesn't look good at all
performance-wise. Before PR:

julia> @benchmark measurement(1, 2,)
BenchmarkTools.Trial: 10000 samples with 999 evaluations.
 Range (min … max):   7.706 ns …  1.856 μs  ┊ GC (min … max):  0.00% … 99.28%
 Time  (median):      9.483 ns              ┊ GC (median):     0.00%
 Time  (mean ± σ):   12.307 ns ± 43.490 ns  ┊ GC (mean ± σ):  10.97% ±  3.13%

  ▄▅▃▂██▅▃▂▁                           ▁ ▁▂▂▁▁                ▂
  ███████████▇▆▅▆▅▆▅▅▅▅▆▄▄▃▄▄▃▄▅▅▅▆▇▇██████████▆▆▄▆▆▅▆▆▆▆▆▆▆▅ █
  7.71 ns      Histogram: log(frequency) by time        28 ns <

 Memory estimate: 48 bytes, allocs estimate: 1.

After PR:

julia> @benchmark measurement(1, 2)
BenchmarkTools.Trial: 10000 samples with 192 evaluations.
 Range (min … max):  511.146 ns …  19.476 μs  ┊ GC (min … max): 0.00% … 96.25%
 Time  (median):     544.625 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   577.264 ns ± 528.867 ns  ┊ GC (mean ± σ):  2.86% ±  3.05%

  ▆▅▆▆▆█▅▃▂▂▂▃▂▂▂▂▃▃▁▁▁▂▁                                       ▂
  █████████████████████████▇▇█▇█▇▇▇▆▇▇▇▇█▇▇▆▅▆▆▆▇▆▄▃▄▅▆▅▆▅▅▆▄▁▅ █
  511 ns        Histogram: log(frequency) by time        908 ns <

 Memory estimate: 256 bytes, allocs estimate: 9.

I'm not really convinced it's worth killing performance this badly, the tag has
a very minor role.

Also the benchmarks run in this PR confirm the bad effect on performance: https://github.com/JuliaPhysics/Measurements.jl/blob/3191a32b3043eb15d120de82371b2cfab33b077e/2022/03/28/214322/result.md#results

@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2022

Codecov Report

Merging #118 (ea061f8) into master (a5787bd) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
- Coverage   95.57%   95.56%   -0.02%     
==========================================
  Files          13       13              
  Lines         746      744       -2     
==========================================
- Hits          713      711       -2     
  Misses         33       33              
Impacted Files Coverage Δ
src/Measurements.jl 87.09% <100.00%> (-0.79%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@giordano
Copy link
Member Author

giordano commented Apr 2, 2022

With the atomic tag counter I get:

julia> @benchmark measurement(1, 2)
BenchmarkTools.Trial: 10000 samples with 999 evaluations.
 Range (min  max):  12.174 ns   2.005 μs  ┊ GC (min  max): 0.00%  98.94%
 Time  (median):     15.165 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   17.692 ns ± 48.266 ns  ┊ GC (mean ± σ):  8.53% ±  3.12%

  ▅▅▅▄▄▃▂█▇█▇▃▂  ▁                   ▁▁▂▂▃▂▁▂▁▁▁              ▂
  ██████████████▇█▇█▇▅▇▇▆▇▄▅▅▅▆▅▁▄▆▆▇████████████▇▅▆▆▆▇▇▇▇▇▇▇ █
  12.2 ns      Histogram: log(frequency) by time      32.6 ns <

 Memory estimate: 48 bytes, allocs estimate: 1.

Still slower than before, but the penalty is much more reasonable. Benchmarks: https://github.com/JuliaPhysics/Measurements.jl/blob/8b5405cd63498b2c9da5e4446eaea350bb1806b2/2022/04/02/174030/result.md#results

@giordano giordano changed the title Use task-local storage for the tag, instead of thread-local Use atomic tag counter Apr 2, 2022
@giordano giordano marked this pull request as ready for review August 31, 2022 22:38
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