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

Remove multi-threading support #93

Merged
merged 1 commit into from
Mar 22, 2023

Conversation

danielocfb
Copy link
Collaborator

We have some limited support for doing DWARF parsing with multiple threads in debug_info_parse_symbols(), but it is not currently used anywhere but tests/benchmarks. I adjusted the resolver to use four instead of only a single thread but performance of our end-to-end test was unaffected:

  dwarf/dwarf :: lookup_end_to_end
                          time:   [38.452 ms 38.729 ms 39.055 ms]
                          change: [+0.4597% +1.5175% +2.5883%] (p = 0.00 < 0.02)
                          Change within noise threshold.
  Found 50 outliers among 500 measurements (10.00%)
    24 (4.80%) high mild
    26 (5.20%) high severe
  dwarf/dwarf :: symbolize_end_to_end
                          time:   [7.8156 ms 7.8725 ms 7.9420 ms]
                          change: [-0.7813% +0.0273% +0.9428%] (p = 0.95 > 0.02)
                          No change in performance detected.
  Found 43 outliers among 500 measurements (8.60%)
    16 (3.20%) high mild
    27 (5.40%) high severe

This change removes this multi-threaded code as it was not actually usable by users and consumed an additional dependency. We can resurrect it if truly needed, but APIs should likely be designed differently and not spawn threads internally.

We have some limited support for doing DWARF parsing with multiple
threads in debug_info_parse_symbols(), but it is not currently used
anywhere but tests/benchmarks. I adjusted the resolver to use four
instead of only a single thread but performance of our end-to-end test
was unaffected:
  dwarf/dwarf :: lookup_end_to_end
                          time:   [38.452 ms 38.729 ms 39.055 ms]
                          change: [+0.4597% +1.5175% +2.5883%] (p = 0.00 < 0.02)
                          Change within noise threshold.
  Found 50 outliers among 500 measurements (10.00%)
    24 (4.80%) high mild
    26 (5.20%) high severe
  dwarf/dwarf :: symbolize_end_to_end
                          time:   [7.8156 ms 7.8725 ms 7.9420 ms]
                          change: [-0.7813% +0.0273% +0.9428%] (p = 0.95 > 0.02)
                          No change in performance detected.
  Found 43 outliers among 500 measurements (8.60%)
    16 (3.20%) high mild
    27 (5.40%) high severe

This change removes this multi-threaded code as it was not actually
usable by users and consumed an additional dependency. We can resurrect
it if truly needed, but APIs should likely be designed differently and
not spawn thread internally.

Signed-off-by: Daniel Müller <deso@posteo.net>
@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.11 ⚠️

Comparison is base (ef679c8) 68.84% compared to head (acc268e) 68.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
- Coverage   68.84%   68.74%   -0.11%     
==========================================
  Files          13       13              
  Lines        4260     4204      -56     
==========================================
- Hits         2933     2890      -43     
+ Misses       1327     1314      -13     
Impacted Files Coverage Δ
src/dwarf/parser.rs 71.56% <100.00%> (-0.37%) ⬇️
src/dwarf/resolver.rs 83.25% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@anakryiko anakryiko left a comment

Choose a reason for hiding this comment

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

Discussed this offline. It's surprising multi-threading doesn't help, so we'll need to circle back to this at some point. For now let's record an issue to add multi-threaded support later on to not forget.

@danielocfb danielocfb merged commit 92c1d00 into libbpf:master Mar 22, 2023
@danielocfb danielocfb deleted the topic/no-crossbeam branch March 22, 2023 17:44
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.

None yet

3 participants