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

perf: Optimize cloning of Context since it is immutable #2861

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bantonsson
Copy link
Contributor

@bantonsson bantonsson commented Mar 25, 2025

Changes

This PR optimizes Context cloning and attach. It tries to mitigate the performance impact of #2821 on those operations.

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)

Copy link

codecov bot commented Mar 25, 2025

Codecov Report

Attention: Patch coverage is 95.83333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 81.1%. Comparing base (99cb67d) to head (9a9e5f6).

Files with missing lines Patch % Lines
opentelemetry/src/context.rs 95.3% 3 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2861   +/-   ##
=====================================
  Coverage   81.1%   81.1%           
=====================================
  Files        124     124           
  Lines      23927   23961   +34     
=====================================
+ Hits       19410   19441   +31     
- Misses      4517    4520    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bantonsson bantonsson force-pushed the ban/context-suppression branch 3 times, most recently from d79a782 to dc038f9 Compare March 25, 2025 15:36
@scottgerring scottgerring reopened this Mar 26, 2025
@scottgerring
Copy link
Contributor

👷 build bot output from this run:

name baseDuration changesDuration difference
context_attach/nested_cx/empty_cx 31.7±0.14ns 19.1±0.27ns -40
context_attach/nested_cx/single_value_cx 33.7±0.22ns 19.4±0.44ns -42
context_attach/nested_cx/span_cx 33.0±0.29ns 19.4±0.30ns -41
context_attach/out_of_order_cx_drop/empty_cx 38.7±0.18ns 19.8±0.19ns -49
context_attach/out_of_order_cx_drop/single_value_cx 40.1±0.13ns 20.8±0.48ns -48
context_attach/out_of_order_cx_drop/span_cx 39.7±0.22ns 20.8±0.32ns -48
context_attach/single_cx/empty_cx 18.1±0.22ns 12.5±0.31ns -31
context_attach/single_cx/single_value_cx 17.6±0.08ns 12.5±0.13ns -29
context_attach/single_cx/span_cx 17.3±0.13ns 12.5±0.25ns -28

@bantonsson bantonsson force-pushed the ban/context-suppression branch from dc038f9 to 06eb4a1 Compare March 27, 2025 08:10
@bantonsson
Copy link
Contributor Author

These are the benchmark numbers from this run:

And yes, the suppression check is slower, but on the other hand context attach and cloning is way faster.

name baseDuration changesDuration difference
context/has_active_span/in-cx/alt 8.4±0.01ns 8.4±0.02ns 0.0
context/has_active_span/in-cx/spec 4.4±0.06ns 4.4±0.07ns 0.0
context/has_active_span/no-cx/alt 8.4±0.02ns 8.4±0.01ns 0.0
context/has_active_span/no-cx/spec 4.4±0.09ns 4.4±0.05ns 0.0
context/has_active_span/no-sdk/alt 8.4±0.03ns 8.4±0.01ns 0.0
context/has_active_span/no-sdk/spec 4.4±0.10ns 4.4±0.06ns 0.0
context/is_recording/in-cx/alt 4.7±0.06ns 4.7±0.06ns 0.0
context/is_recording/in-cx/spec 6.6±0.08ns 6.6±0.07ns 0.0
context/is_recording/no-cx/alt 4.7±0.05ns 4.7±0.04ns 0.0
context/is_recording/no-cx/spec 6.5±0.06ns 6.6±0.15ns 0.0
context/is_recording/no-sdk/alt 4.7±0.07ns 4.7±0.07ns 0.0
context/is_recording/no-sdk/spec 6.5±0.07ns 6.5±0.09ns 0.0
context/is_sampled/in-cx/alt 8.7±0.02ns 8.7±0.03ns 0.0
context/is_sampled/in-cx/spec 4.6±0.06ns 4.6±0.07ns 0.0
context/is_sampled/no-cx/alt 8.7±0.04ns 8.7±0.02ns 0.0
context/is_sampled/no-cx/spec 4.7±0.06ns 4.7±0.09ns 0.0
context/is_sampled/no-sdk/alt 8.7±0.03ns 8.7±0.03ns 0.0
context/is_sampled/no-sdk/spec 4.7±0.05ns 4.7±0.07ns 0.0
context_attach/nested_cx/empty_cx 44.6±0.31ns 19.2±0.80ns -57
context_attach/nested_cx/single_value_cx 44.6±0.30ns 19.6±0.42ns -56
context_attach/nested_cx/span_cx 44.6±0.44ns 19.5±0.20ns -56
context_attach/out_of_order_cx_drop/empty_cx 38.5±0.40ns 20.0±0.41ns -48
context_attach/out_of_order_cx_drop/single_value_cx 38.6±0.43ns 21.2±0.42ns -45
context_attach/out_of_order_cx_drop/span_cx 38.6±0.45ns 21.1±0.41ns -45
context_attach/single_cx/empty_cx 22.8±0.20ns 10.3±0.19ns -55
context_attach/single_cx/single_value_cx 22.0±0.22ns 11.0±0.20ns -50
context_attach/single_cx/span_cx 22.0±0.25ns 11.0±0.11ns -50
telemetry_suppression/enter_telemetry_suppressed_scope 28.4±0.09ns 24.5±0.96ns -14
telemetry_suppression/is_current_telemetry_suppressed_false 1.3±0.02ns 1.9±0.03ns +47
telemetry_suppression/is_current_telemetry_suppressed_true 1.3±0.01ns 2.1±0.04ns +68
telemetry_suppression/normal_attach 28.5±0.16ns 10.0±0.18ns -65

@bantonsson bantonsson marked this pull request as ready for review March 27, 2025 10:28
@bantonsson bantonsson requested a review from a team as a code owner March 27, 2025 10:28
@bantonsson bantonsson force-pushed the ban/context-suppression branch 4 times, most recently from e3aca49 to ad3ad40 Compare March 28, 2025 09:14
@bantonsson bantonsson marked this pull request as draft March 28, 2025 14:58
@bantonsson
Copy link
Contributor Author

This is still a draft until #2870 has been merged and all benchmarks are run properly.

@bantonsson bantonsson force-pushed the ban/context-suppression branch from ad3ad40 to 9a9e5f6 Compare March 31, 2025 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants