Skip to content

Make LegacyPropagator list sort linear - #132373

Merged
tarekgh merged 1 commit into
dotnet:mainfrom
martincostello:improve-reverse-efficiency
Aug 21, 2026
Merged

Make LegacyPropagator list sort linear#132373
tarekgh merged 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 sizeOld (Insert)New (Add+Reverse)RatioAllocated (both)
3~11 B135.6 ns134.6 ns1.0x528 B
10~39 B499.0 ns518.7 ns1.0x1,912 B
50~199 B2,832.3 ns2,481.9 ns1.1x8,936 B
500~2 KB56.4 µs25.3 µs2.2x87,744 B
2,000~7.8 KB556.9 µs113.2 µs4.9x360,948 B
2,048 (8 KB)8 KB519.9 µs65.0 µs8.0x288.25 KB
4,096 (16 KB)16 KB1,992.0 µs229.4 µs8.7x576.28 KB
8,192 (32 KB)32 KB8,298.8 µs916.7 µs9.1x1,152.45 KB
16,384 (64 KB)64 KB32,119.9 µs2,696.4 µs11.9x2,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>
CopilotAI lite review requested due to automatic review settings August 16, 2026 14:36
@dotnet-policy-servicedotnet-policy-serviceBot 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.

CopilotAI 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.

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

@tarekgh

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

@tarekghtarekgh 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
MemberAuthor

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-servicedotnet-policy-serviceBot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 16, 2026
@tarekgh

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
MemberAuthor

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

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.

@tarekgh

Copy link
Copy Markdown
Member

/ba-g the failures are unrelated

@tarekghtarekgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks @martincostello

@tarekghtarekgh added this to the 12.0.0 milestone Aug 21, 2026
@tarekgh
tarekgh merged commit 56b3798 into dotnet:mainAug 21, 2026
94 of 97 checks passed
@martincostello
martincostello deleted the improve-reverse-efficiency branch August 22, 2026 06:27
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Diagnostics.Tracingcommunity-contributionIndicates 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

@martincostello@tarekgh