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

make Tracer::Clone cheaper (another 10% improvement to start-end-span/never-sample) #1093

Merged
merged 1 commit into from
Jun 6, 2023
Merged

Conversation

shaun-cox
Copy link
Contributor

Fixes #1092

This is more of a workaround in that I think there is still more work to be done inside InstrumentationLibrary type itself, but waiting for more discussion.

Changes

  • change Tracer to hold an Arc rather than the whole 120 byte instance, so that Cloning is way cheaper
  • IMO, the Arc should move internal to InstrumentationLibrary itself, but that is a bigger API-breaking change.
start-end-span/always-sample
                        time:   [508.72 ns 523.52 ns 536.82 ns]
                        change: [-6.7810% -1.9044% +3.2262%] (p = 0.46 > 0.05)
                        No change in performance detected.
start-end-span/never-sample
                        time:   [120.16 ns 120.48 ns 120.81 ns]
                        change: [-10.510% -10.308% -10.116%] (p = 0.00 < 0.05)
                        Performance has improved.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

- change Tracer to hold an Arc<InstrumentationLibrary> rather than
  the whole 120 byte instance, so that Cloning is way cheaper
- IMO, the Arc should move internal to InstrumentationLibrary itself, but
  that is a bigger API-breaking change.

start-end-span/always-sample
                        time:   [508.72 ns 523.52 ns 536.82 ns]
                        change: [-6.7810% -1.9044% +3.2262%] (p = 0.46 > 0.05)
                        No change in performance detected.
start-end-span/never-sample
                        time:   [120.16 ns 120.48 ns 120.81 ns]
                        change: [-10.510% -10.308% -10.116%] (p = 0.00 < 0.05)
                        Performance has improved.
@shaun-cox shaun-cox requested a review from a team June 5, 2023 19:28
@shaun-cox shaun-cox changed the title make Tracer::Clone cheaper #1092 (another 10% improvement to start-end-span/never-sample) make Tracer::Clone cheaper (another 10% improvement to start-end-span/never-sample) Jun 5, 2023
@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Patch coverage: 100.0% and no project coverage change.

Comparison is base (800b9e3) 50.7% compared to head (8af3715) 50.7%.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1093   +/-   ##
=====================================
  Coverage   50.7%   50.7%           
=====================================
  Files        165     165           
  Lines      19801   19808    +7     
=====================================
+ Hits       10051   10059    +8     
+ Misses      9750    9749    -1     
Impacted Files Coverage Δ
opentelemetry-sdk/src/trace/provider.rs 90.0% <100.0%> (+0.7%) ⬆️
opentelemetry-sdk/src/trace/tracer.rs 93.5% <100.0%> (+<0.1%) ⬆️

... and 1 file with indirect coverage changes

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

@jtescher jtescher merged commit 18f6c01 into open-telemetry:main Jun 6, 2023
@shaun-cox shaun-cox deleted the workaround_1092 branch June 6, 2023 16:16
@lalitb lalitb mentioned this pull request Jun 27, 2023
4 tasks
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.

Tracer::Clone is at least 30% of the cost of Tracer::start, because of InstrumentationLibrary
3 participants