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 property expansion #6128

Merged
merged 8 commits into from Mar 3, 2021
Merged

Conversation

@ladipro
Copy link
Contributor

@ladipro ladipro commented Feb 7, 2021

Fixes #6063

Context

Property expansion has been identified as one of the hot spots in project evaluation and ExpandPropertiesLeaveEscaped alone
accounts for almost 20% of overall evaluation time of simple projects.

Changes Made

Several optimizations have been made to the calltree under ExpandPropertiesLeaveEscaped. Notably:

  • Unnecessary and counter-productive string pinning has been removed.
  • A slow strchr-like function has been replaced with a simple call to String.IndexOf.
  • Series of ifs have been replaced with a switch.
  • Allocation of List<object> results has been eliminated.
  • Allocation of temporary substrings extracted from the expression has been eliminated.

The combined performance win is 10% in ExpandPropertiesLeaveEscaped, so close to 2% for evaluation overall.

Testing

Expander is well covered by existing unit tests.

Notes

Please review commit by commit for easier to read diffs.

Copy link

@donJoseLuis donJoseLuis left a comment

Cool stuff. The only thing to think about is if in a disposable type, you wish to check if instances have already been disposed prior to executing public method bodies.

src/Build/Evaluation/Expander.cs Show resolved Hide resolved
src/Build/Evaluation/Expander.cs Outdated Show resolved Hide resolved
src/Build/Evaluation/Expander.cs Show resolved Hide resolved
src/Build/Evaluation/Expander.cs Outdated Show resolved Hide resolved
@ladipro ladipro force-pushed the ladipro:6063-faster-property-expansion branch from 483dd9e to 2bd3656 Feb 8, 2021
Copy link
Contributor Author

@ladipro ladipro left a comment

Thank you @donJoseLuis. I have addressed your feedback and updated the PR.

src/Build/Evaluation/Expander.cs Show resolved Hide resolved
src/Build/Evaluation/Expander.cs Show resolved Hide resolved
src/Build/Evaluation/Expander.cs Outdated Show resolved Hide resolved
src/Build/Evaluation/Expander.cs Outdated Show resolved Hide resolved
@Forgind
Forgind approved these changes Feb 8, 2021
src/Build/Evaluation/Expander.cs Show resolved Hide resolved
src/Shared/FileUtilities.cs Outdated Show resolved Hide resolved
src/Shared/FileUtilities.cs Show resolved Hide resolved
src/Build/Evaluation/Expander.cs Show resolved Hide resolved
src/Build/Evaluation/Expander.cs Show resolved Hide resolved
src/Build/Evaluation/Expander.cs Show resolved Hide resolved
@Forgind
Copy link
Member

@Forgind Forgind commented Feb 8, 2021

I feel like there was a rumor of eventually replacing Expander entirely, but I might be misremembering.

Copy link
Member

@BenVillalobos BenVillalobos left a comment

Looks great! Did you get a sense of how many properties were being expanded that only had a single object?

@ladipro ladipro force-pushed the ladipro:6063-faster-property-expansion branch from 2bd3656 to 6b55daa Feb 8, 2021
Copy link
Contributor Author

@ladipro ladipro left a comment

I feel like there was a rumor of eventually replacing Expander entirely, but I might be misremembering.

That would be exciting (and scary at the same time). Any pointers to such plans would be welcome!

src/Build/Evaluation/Expander.cs Show resolved Hide resolved
src/Shared/FileUtilities.cs Show resolved Hide resolved
@Forgind
Copy link
Member

@Forgind Forgind commented Feb 8, 2021

I looked around, and what I found suggested no concrete plans—just an idea:
#5226 (comment)

I was probably a little premature here 🙂

@ladipro
Copy link
Contributor Author

@ladipro ladipro commented Feb 8, 2021

Looks great! Did you get a sense of how many properties were being expanded that only had a single object?

That's a great question. When building a .NET Core hello world console app, out of 5632 invocations of ExpandPropertiesLeaveTypedAndEscaped,

  • 3002 don't expand any property (the string is a literal),
  • 2261 expand to just one property (e.g. "$(Prop)"), out of which 52 expands to a non-string value,
  • 122 expand to one property followed by a literal (e.g. "$(Prop)SomeString"),
  • 22 expand to one property prepended by a literal (e.g. "SomeString$(Prop)"),
  • 46 expand to one property followed and prepended by string literals (e.g. "Some$(Prop)String"),
  • 182 expand multiple properties (e.g. "$(Prop)and$(AnotherProp)").
@ladipro ladipro force-pushed the ladipro:6063-faster-property-expansion branch from 6b55daa to 86b65b7 Feb 10, 2021
Copy link

@donJoseLuis donJoseLuis left a comment

thanks for updating to check if instances are disposed in public members.

@rokonec rokonec merged commit cbca164 into dotnet:master Mar 3, 2021
7 checks passed
7 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210210.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
ladipro added a commit that referenced this pull request May 6, 2021
…urn null (#6414)

Fixes #6413

### Context

This is a regression introduced in #6128. MSBuild crashes when evaluating a project where a property function returns null and its result is concatenated with another non-empty value.

### Changes Made

Add a null check.

### Testing

Fixed and extended the relevant test case.
rdipardo added a commit to rdipardo/Fornax.Seo that referenced this pull request Jul 5, 2021
.NET SDK 5.0.300 ships with MSBuild 16.10, which optimizes away the
implicit expansion of quoted property lists [*]:

    FSC : warning FS0203: Invalid warning number '3218 3390'

Fortunately this old (non-portable) workaround still works:

    dotnet/sdk#8792 (comment)

----
[*] dotnet/msbuild#6128
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.

6 participants