-
Notifications
You must be signed in to change notification settings - Fork 287
Benchmark test for GetFullPath with redundant segments #1394
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
Conversation
src/benchmarks/micro/libraries/System.Runtime.Extensions/Perf.Path.cs
Outdated
Show resolved
Hide resolved
|
I think this test should have more typical cases compared to these edge cases. (Or at least include them.) I suspect |
|
(And I know we already have a few other tests, but its not clear to me that these give a ton of benefit over those.) |
|
One other thought about these tests. It looks like the expected improvement is ~2%, right? In general our system isn't going to flag that as a regression if we lost this optimization somehow. That's typically below the noise threshold we hold. @DrewScoggins can opine some here. |
|
@adamsitnik @jozkee @jkotas I ran these benchmarks, and they are giving me a different result than what we saw with the |
|
Updated the description to include Windows benchmark tests too. |
| private readonly string _testPath500 = PerfUtils.CreateString(500); | ||
| private readonly string _testPath1000 = PerfUtils.CreateString(1000); | ||
| private readonly string _testPathNoRedundantSegments = "/home/user/runtime/src/coreclr/runtime/src/libraries/System.Private.CoreLib/src/System/IO/Path.cs"; | ||
| private readonly string _testPathWithRedundantSegments = "/home/user/runtime/src/coreclr/runtime/src/libraries/System.Private.CoreLib/src/System/IO/..//./Path.cs"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider adding rooted paths like C:\ProgramData and paths with flipped separators?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a conversation with Adam a few minutes ago, we touched the topic of Windows, precisely.
I added a new commit, can you please take a look? It has comments describing the purpose of each new path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the flipped separators? It looks to me like a common case that is worth checking for regression, at least on windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adamsitnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @carlossanlop !
Adding a benchmark test to compare results of calling
Path.GetFullPathusing paths that contain redundant segments.Related to: Add APIs that remove redundant segments from paths #37939
Background
The
GetFullPathcurrently calls an internal method that does not handle all Windows edge cases. Now that we want to make this method public, those edge cases need to be handled, and require adding extra functionality that will affect the original performance and allocations.The Unix code also changed a bit.
Results
UPDATE: I am posting new results to match the suggested changes in this PR (49a3ee5) and in the runtime PR (dotnet/runtime@3b72390).
I added two benchmarks with two long paths. One has no redundant segments, the other does.
Code
Ubuntu benchmarks
Results before
Results after
Results comparison
Windows benchmarks
Results before
Results after
Results comparison