Optimize EngineFileUtilities.GetFileList#6227
Merged
benvillalobos merged 6 commits intodotnet:masterfrom Mar 11, 2021
Merged
Conversation
a1f33c2 to
1117dac
Compare
Forgind
approved these changes
Mar 5, 2021
Contributor
There was a problem hiding this comment.
You took out the color from the original! But that's ok 😉
src/Shared/EscapingUtilities.cs
Outdated
src/Shared/EscapingUtilities.cs
Outdated
Contributor
There was a problem hiding this comment.
We want something that looks like %3f or whatever, and that takes three characters. This means no need for verifying that there are enough characters afterwards.
Member
There was a problem hiding this comment.
Ah, index + 1 means 1 less character to look at ;)
Contributor
There was a problem hiding this comment.
Well, we also know that index is the index of a %, so using index instead would result in saying index = index; ad infinitum.
Member
Author
There was a problem hiding this comment.
I tweaked the expression to be more readable and added comments.
cdmihai
approved these changes
Mar 5, 2021
benvillalobos
approved these changes
Mar 6, 2021
1117dac to
5f4f7c2
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Fixes #6061
Context
EngineFileUtilities.GetFileListhas been identified as one of the performance bottlenecks when building .NET Core projects. Since this code is called as part of project evaluation, it directly impacts Visual Studio performance as well, especially solution load.Changes Made
Tweaked the code under
GetFileList:Testing
Existing unit tests for correctness, ETL traces for performance.
This change together with #6151 is showing about 30% less time spent in this particular function when building a Hello World .NET Core project with the Framework version of MSBuild. It's an OK win for project evaluation perf but translates to less than 1 ms of total command line build time.
Notes
This PR is small but still recommended to be reviewed commit by commit.