Skip to content
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

Optimize EngineFileUtilities.GetFileList #6227

Merged
merged 6 commits into from Mar 11, 2021

Conversation

@ladipro
Copy link
Contributor

@ladipro ladipro commented Mar 4, 2021

Fixes #6061

Context

EngineFileUtilities.GetFileList has 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:

  • Smarter order of wildcard checks to optimize for the common case.
  • Optimized hex number parsing and wildcard detection.

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.

@ladipro ladipro force-pushed the ladipro:6061-optimize-GetFileListEscaped branch 2 times, most recently from a1f33c2 to 1117dac Mar 5, 2021
@Forgind
Forgind approved these changes Mar 5, 2021
Copy link
Member

@Forgind Forgind left a comment

LGTM, thanks!

{
return false;
}

// If the item's Include has both escaped wildcards and real wildcards, then it's

This comment has been minimized.

@Forgind

Forgind Mar 5, 2021
Member

You took out the color from the original! But that's ok 😉

Copy link
Member

@BenVillalobos BenVillalobos left a comment

Mostly LGTM

string hexString = escapedString.Substring(indexOfPercent + 1, 2);
char unescapedCharacter = (char)int.Parse(hexString, System.Globalization.NumberStyles.HexNumber,
CultureInfo.InvariantCulture);
char unescapedCharacter = (char)((digit1 << 4) + digit2);

This comment has been minimized.

}
index = escapedString.IndexOf('%', index + 1, escapedString.Length - index - 3);

This comment has been minimized.

@BenVillalobos

BenVillalobos Mar 5, 2021
Member

Why subtract 3 here?

This comment has been minimized.

@Forgind

Forgind Mar 6, 2021
Member

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.

This comment has been minimized.

@BenVillalobos

BenVillalobos Mar 6, 2021
Member

Ah, index + 1 means 1 less character to look at ;)

This comment has been minimized.

@Forgind

Forgind Mar 6, 2021
Member

Well, we also know that index is the index of a %, so using index instead would result in saying index = index; ad infinitum.

This comment has been minimized.

@ladipro

ladipro Mar 9, 2021
Author Contributor

I tweaked the expression to be more readable and added comments.

@cdmihai
cdmihai approved these changes Mar 5, 2021
@ladipro ladipro force-pushed the ladipro:6061-optimize-GetFileListEscaped branch from 1117dac to 5f4f7c2 Mar 9, 2021
@BenVillalobos BenVillalobos merged commit e06c993 into dotnet:master Mar 11, 2021
7 checks passed
7 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210309.1 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants