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

Conversation

ladipro
Copy link
Member

@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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 6063-faster-property-expansion branch from 483dd9e to 2bd3656 Compare February 8, 2021 14:20
Copy link
Member Author

@ladipro ladipro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 6063-faster-property-expansion branch from 2bd3656 to 6b55daa Compare February 8, 2021 22:47
Copy link
Member Author

@ladipro ladipro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
Member Author

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 6063-faster-property-expansion branch from 6b55daa to 86b65b7 Compare February 10, 2021 12:05
Copy link

@donJoseLuis donJoseLuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 10, 2021
@rokonec rokonec merged commit cbca164 into dotnet:master Mar 3, 2021
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
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExpandPropertiesLeaveEscaped using ReuseableStringBuilder 50% slower than OpportunisticIntern
6 participants