Skip to content

Make LegacyPropagator list sort linear - #132373

Open
martincostello wants to merge 1 commit into
dotnet:mainfrom
martincostello:improve-reverse-efficiency
Open

Make LegacyPropagator list sort linear#132373
martincostello wants to merge 1 commit into
dotnet:mainfrom
martincostello:improve-reverse-efficiency

Conversation

@martincostello

Copy link
Copy Markdown
Member

Replace O(n^2) behaviour with O(n) to reverse the list of keys parsed from the baggage header by LegacyPropagator, matching the implementation in W3CPropagator.

Also adds a test to verify the ordering.

Impact in the default case is minimised by Kestrel and IIS' HTTP request header limits (8KB and 16KB) and the legacy propagator being opt-in in .NET 10+.

N (entries) Header size Old (Insert) New (Add+Reverse) Ratio Allocated (both)
3 ~11 B 135.6 ns 134.6 ns 1.0x 528 B
10 ~39 B 499.0 ns 518.7 ns 1.0x 1,912 B
50 ~199 B 2,832.3 ns 2,481.9 ns 1.1x 8,936 B
500 ~2 KB 56.4 µs 25.3 µs 2.2x 87,744 B
2,000 ~7.8 KB 556.9 µs 113.2 µs 4.9x 360,948 B
2,048 (8 KB) 8 KB 519.9 µs 65.0 µs 8.0x 288.25 KB
4,096 (16 KB) 16 KB 1,992.0 µs 229.4 µs 8.7x 576.28 KB
8,192 (32 KB) 32 KB 8,298.8 µs 916.7 µs 9.1x 1,152.45 KB
16,384 (64 KB) 64 KB 32,119.9 µs 2,696.4 µs 11.9x 2,304.73 KB

Replace O(n^2) behaviour with O(n) to reverse the list of keys parsed from the baggage header by `LegacyPropagator`, matching the implementation in `W3CPropagator`.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings August 16, 2026 14:36
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Aug 16, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Improves LegacyPropagator baggage extraction performance by avoiding repeated List.Insert(0, …) (quadratic behavior) and instead appending entries then reversing once, aligning the ordering behavior with W3CPropagator’s extraction logic. Updates existing propagator tests so baggage ordering expectations are exercised with multiple entries.

Changes:

  • Replace per-entry Insert(0, …) with Add(…) plus a single Reverse() at the end of LegacyPropagator.TryExtractBaggage.
  • Expand legacy-propagator test inputs to include multiple baggage entries so reverse-order behavior is actually validated.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/LegacyPropagator.cs Switches baggage list construction to Add + one-time Reverse() to avoid O(n²) behavior while preserving ordering semantics.
src/libraries/System.Diagnostics.DiagnosticSource/tests/PropagatorTests.cs Adds additional baggage entries to legacy propagator test cases so ordering/reversal is meaningfully asserted.

@tarekgh

tarekgh commented Aug 16, 2026

Copy link
Copy Markdown
Member

I don't think we need to change the legacy propagator and risk any app compatibility issues. We introduced the W3CPropagator to be more compliant with the specifications, and it is now the default.

Additionally, making the legacy propagator behave like the W3CPropagator is not a goal. Otherwise, there would have been no reason to introduce a separate W3CPropagator in the first place.

Please let's know why not using W3CPropagator is enough.

CC @noahfalk

@tarekgh tarekgh added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 16, 2026
@martincostello

martincostello commented Aug 16, 2026

Copy link
Copy Markdown
Member Author

The goal is purely to improve the performance for users who may still be using the legacy implementation for whatever reason, as noted in the description. There should be zero observable difference other than time.

@dotnet-policy-service dotnet-policy-service Bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 16, 2026
@tarekgh

tarekgh commented Aug 16, 2026

Copy link
Copy Markdown
Member

@martincostello, thanks. Did you encounter any complaints about the performance there, or are you just doing this proactively? My point is this is really legacy and not the default anymore. The benefit is not that worth to touch the code there.

@martincostello

Copy link
Copy Markdown
Member Author

No, I had Copilot look at the o11y-related code to see if there was anything it could find that was a low-cost optimisation that could be made, which surfaced #132368 and this.

The change seemed a no-brainer to me when I reviewed the findings considering that in terms of implementation, disregarding any propagation differences between W3C and the previous implementation of how baggage propagates, it's the same (insert in order, then reverse at the end).

@tarekgh

tarekgh commented Aug 16, 2026

Copy link
Copy Markdown
Member

Okay, let's wait for the snap for RC1 to be done, and then I can review and merge it after that, including #132368.

@martincostello
martincostello marked this pull request as ready for review August 16, 2026 17:35
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Diagnostics.Tracing community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants