Skip to content

Fix route analyzer performance with highly concatenated strings #54479

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
Mar 22, 2024
Merged

Fix route analyzer performance with highly concatenated strings #54479

merged 16 commits into from
Mar 22, 2024

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Mar 11, 2024 •
edited
Loading

Fixes #54293
Fixes #53899

Route analyzer introduced in .NET 8 is causing very long build times if some projects. Highly concatenated strings caused a lot of time to be spent in HasLanguageComment.

The PR:

This PR will likely be backported to 8.0.

@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Mar 11, 2024
@JamesNK JamesNK mentioned this pull request Mar 11, 2024
Closed
@JamesNK JamesNK changed the title Add test for route analyzer performance issue Improve route analyzer performance with strings that have many concatenations Mar 12, 2024
@JamesNK JamesNK marked this pull request as ready for review March 12, 2024 01:37
@JamesNK
Copy link
Member Author

JamesNK commented Mar 12, 2024

@jjonescz, @CyrusNajmabadi, @cston Roslyn expert opinion on this fix would be great.

@JamesNK JamesNK changed the title Improve route analyzer performance with strings that have many concatenations Improve route analyzer performance with highly concatenated strings Mar 12, 2024
@JamesNK
Copy link
Member Author

JamesNK commented Mar 18, 2024 •
edited
Loading

I've greatly simplified this PR.

The new workaround is to avoid calling GetLeadingTrivia() on every level of a nested binary expression. Instead, check the first parent of the string node and then resume checking once out of the nested binary expression tree.

Detection behavior doesn't 100% emulate embedded language classification by Roslyn, but I've found some odd quirks when mixing multiple lang comments in a string concat tree from Roslyn anyway.

The analyzer behavior here is good enough for normal usage. It's the best balance I've found between good behavior, good performance, and minimal code change.

@CyrusNajmabadi
Copy link
Member

image

@CyrusNajmabadi
Copy link
Member

@JamesNK i just went with dotnet/roslyn#72620 as a simple approach.

@JamesNK
Copy link
Member Author

JamesNK commented Mar 21, 2024 •
edited
Loading

@JamesNK i just went with dotnet/roslyn#72620 as a simple approach.

Perfect, I like it.

Trying to match Roslyn behavior while getting good perf was wrapping this PR up in knots. Great that Roslyn just simplified, and this PR can copy it.

Performance with this change is good - 0.09s after compared to 5s before

@JamesNK
Copy link
Member Author

JamesNK commented Mar 21, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

mitchdenny
mitchdenny approved these changes Mar 21, 2024
View reviewed changes
Copy link
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

LGTM.

@JamesNK JamesNK force-pushed the jamesnk/routeanalyzer-perf branch from ccc7afe to c00323c Compare March 21, 2024 03:54
@JamesNK JamesNK changed the title Improve route analyzer performance with highly concatenated strings Fix route analyzer performance with highly concatenated strings Mar 21, 2024
@JamesNK
Copy link
Member Author

JamesNK commented Mar 21, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@JamesNK JamesNK enabled auto-merge (squash) March 22, 2024 00:06
@JamesNK JamesNK merged commit ec293ee into main Mar 22, 2024
@JamesNK JamesNK deleted the jamesnk/routeanalyzer-perf branch March 22, 2024 01:39
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview4 milestone Mar 22, 2024
@JamesNK
Copy link
Member Author

JamesNK commented Mar 26, 2024

/backport to release/8.0

@github-actions GitHub Actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/8429842063

@github-actions github-actions bot mentioned this pull request Mar 26, 2024
Merged
11 tasks
@mkArtakMSFT mkArtakMSFT added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework labels Mar 28, 2024
@JamesNK JamesNK mentioned this pull request Apr 2, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjonescz jjonescz jjonescz left review comments

@mitchdenny mitchdenny mitchdenny approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@captainsafia captainsafia Awaiting requested review from captainsafia

+2 more reviewers

@martincostello martincostello martincostello left review comments

@cston cston cston left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc
Projects
None yet
Milestone
9.0-preview4
Development

Successfully merging this pull request may close these issues.

Very slow build - can't build project RoutePatternAnalyzer (ASP0017, ASP0018) performance issue (1.5 minute execution time)
7 participants
@JamesNK @CyrusNajmabadi @mitchdenny @martincostello @jjonescz @cston @mkArtakMSFT