Fix ProcessTags.GetLastPathSegment for some edge cases, add tests
#8103
+49
−12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lucaspimentel
changed the title
lucaspimentel
force-pushed
the
lpimentel/reduce-allocations-in-process-tags
branch
from
97d3939 to
67c69a8
Compare
lucaspimentel
changed the title
lucaspimentel
changed the title
lucaspimentel
force-pushed
the
lpimentel/reduce-allocations-in-process-tags
branch
from
67c69a8 to
e520464
Compare
lucaspimentel
commented
Jan 23, 2026
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8103) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8103) - mean (68ms) : 67, 70
master - mean (77ms) : 74, 80
section Bailout
This PR (8103) - mean (72ms) : 71, 73
master - mean (83ms) : 80, 85
section CallTarget+Inlining+NGEN
This PR (8103) - mean (1,027ms) : 966, 1088
master - mean (1,114ms) : 1035, 1193
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8103) - mean (106ms) : 103, 108
master - mean (123ms) : 119, 128
section Bailout
This PR (8103) - mean (107ms) : 106, 108
master - mean (123ms) : 121, 126
section CallTarget+Inlining+NGEN
This PR (8103) - mean (749ms) : 703, 795
master - mean (813ms) : 743, 883
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8103) - mean (94ms) : 92, 96
master - mean (110ms) : 105, 114
section Bailout
This PR (8103) - mean (95ms) : 94, 96
master - mean (110ms) : 107, 113
section CallTarget+Inlining+NGEN
This PR (8103) - mean (727ms) : 706, 749
master - mean (761ms) : 692, 830
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8103) - mean (92ms) : 90, 94
master - mean (107ms) : 104, 110
section Bailout
This PR (8103) - mean (93ms) : 92, 95
master - mean (108ms) : 105, 112
section CallTarget+Inlining+NGEN
This PR (8103) - mean (647ms) : 630, 664
master - mean (718ms) : 689, 746
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8103) - mean (194ms) : 190, 198
master - mean (197ms) : 189, 205
section Bailout
This PR (8103) - mean (198ms) : 196, 200
master - mean (199ms) : 194, 204
section CallTarget+Inlining+NGEN
This PR (8103) - mean (1,151ms) : 1089, 1212
master - mean (1,149ms) : 1090, 1209
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8103) - mean (278ms) : 273, 283
master - mean (279ms) : 273, 285
section Bailout
This PR (8103) - mean (280ms) : 275, 285
master - mean (280ms) : 274, 287
section CallTarget+Inlining+NGEN
This PR (8103) - mean (939ms) : 885, 993
master - mean (945ms) : 896, 994
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8103) - mean (272ms) : 266, 277
master - mean (273ms) : 265, 281
section Bailout
This PR (8103) - mean (271ms) : 268, 274
master - mean (273ms) : 268, 278
section CallTarget+Inlining+NGEN
This PR (8103) - mean (928ms) : 866, 990
master - mean (932ms) : 878, 987
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8103) - mean (271ms) : 265, 277
master - mean (272ms) : 268, 276
section Bailout
This PR (8103) - mean (272ms) : 267, 276
master - mean (273ms) : 266, 280
section CallTarget+Inlining+NGEN
This PR (8103) - mean (843ms) : 822, 864
master - mean (845ms) : 823, 866
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
lucaspimentel
force-pushed
the
lpimentel/reduce-allocations-in-process-tags
branch
from
9b7d9c9 to
70cdc46
Compare
BenchmarksBenchmark execution time: 2026-01-30 00:23:32 Comparing candidate commit ce3b4d3 in PR branch Found 10 performance improvements and 14 performance regressions! Performance is the same for 152 metrics, 16 unstable metrics. scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorMoreComplexBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs net6.0
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs netcoreapp3.1
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OriginalCharSlice net6.0
scenario:Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery net472
scenario:Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync net472
scenario:Benchmarks.Trace.GraphQLBenchmark.ExecuteAsync net6.0
scenario:Benchmarks.Trace.ILoggerBenchmark.EnrichedLog net6.0
scenario:Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark net6.0
scenario:Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark netcoreapp3.1
scenario:Benchmarks.Trace.Log4netBenchmark.EnrichedLog netcoreapp3.1
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishTwoScopes net6.0
|
lucaspimentel
force-pushed
the
lpimentel/reduce-allocations-in-process-tags
branch
from
70cdc46 to
7d10db6
Compare
bouwkast
reviewed
Jan 27, 2026
…pan, ReadOnlySpan<char> trimChars) is broken
lucaspimentel
force-pushed
the
lpimentel/reduce-allocations-in-process-tags
branch
from
7d10db6 to
6526688
Compare
bouwkast
approved these changes
Jan 28, 2026
| return Path.GetFileName(directoryPath.TrimEnd('\\').TrimEnd('/')); | ||
| return StringUtil.IsNullOrEmpty(directoryPath) ? | ||
| string.Empty : | ||
| new DirectoryInfo(directoryPath).Name; |
Collaborator
There was a problem hiding this comment.
Is creating a new DirectoryInfo the workaround to the TrimEnd bug?
Member
Author
There was a problem hiding this comment.
Not really. We could just use the TrimEnd() that uses string instead of ReadOnlySpan<char> and accept the extra allocations.
But I decided I was worrying to much about just a few allocations that only happen once. Handling paths is quite complicated and DirectoryInfo.Name handles things correctly:
/and\separators- paths with or without trailing separators
- root paths like
/orC:\ - other edge cases like device paths (
\\?\.\etc) and\\server\shareUNC paths - probably other edge cases I haven't even thought of 😅
The drawback is that we have to allocate the DirectoryInfo object and its constructor can also allocate some strings internally. But since this code only runs once, I think it's not a terrible trade-off.
Collaborator
There was a problem hiding this comment.
Nice sounds like a lot easier to handle with DirectoryInfo 👍
lucaspimentel
changed the title
lucaspimentel
changed the title
lucaspimentel
changed the title
ProcessTags.GetLastPathSegment for root paths and other edge cases
lucaspimentel
changed the title
ProcessTags.GetLastPathSegment for root paths and other edge casesProcessTags.GetLastPathSegment for some edge cases, add tests
Member
Author
|
I wanted to give some context on this PR since I did 180 from the original goal 😅 It all started when I saw the After going down that rabbit whole for a while, I realized that handling paths is complex, and I was too focused on reducing just a few small allocations that only happen once. (Yes, every bit counts, but sometimes you have to make trade-offs.) I pivoted the PR to focus less on the allocations (the original goal) and focused instead on a more correct handling of paths by using |
bouwkast
approved these changes
Jan 30, 2026
| return Path.GetFileName(directoryPath.TrimEnd('\\').TrimEnd('/')); | ||
| return StringUtil.IsNullOrEmpty(directoryPath) ? | ||
| string.Empty : | ||
| new DirectoryInfo(directoryPath).Name; |
Collaborator
There was a problem hiding this comment.
Nice sounds like a lot easier to handle with DirectoryInfo 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
Simplified
ProcessTags.GetLastPathSegment()to useDirectoryInfo.Nameand fixed the edge case where root paths (e.g.,/,C:\) would return an empty string.Reason for change
/orC:\becausePath.GetFileName()returns empty for paths ending with a separator, and root paths always logically end with a separator.Implementation details
ProcessTags.cs:
TrimEnd+Path.GetFileNamelogic withnew DirectoryInfo(path).Name, which correctly handles:TagsListandSerializedTagsto usefield ??=pattern (Lazy<T>semantics) to delay initialization until first accessGetSerializedTagsFromList()since it was a one-linerTest coverage
Added unit tests for
GetLastPathSegment:GetLastPathSegment_ReturnsLastDirectory_Windows- Tests Windows paths (rooted, unrooted, with/without trailing separators, root paths, empty, null)GetLastPathSegment_ReturnsLastDirectory_NonWindows- Tests Unix paths (same scenarios)Other details
n/a