-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
SyntaxValueProvider: avoid performance issue with syntax list containing many items #83483
Conversation
Tagging subscribers to this area: @dotnet/area-extensions-logging Issue Details
|
src/libraries/Common/src/Roslyn/SyntaxValueProvider_ForAttributeWithSimpleName.cs
Outdated
Show resolved
Hide resolved
@cston is there is benchmark numbers show the improvement? |
Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices Issue Details
|
From a couple of runs of the added test Those runs were without the fix in dotnet/roslyn#66876 however. In .NET 8 builds that include dotnet/roslyn#66876, there shouldn't be a performance issue iterating a |
foreach (var child in node.ChildNodesAndTokens().Reverse()) | ||
var childNodesAndTokens = node.ChildNodesAndTokens(); | ||
|
||
// Avoid performance issue (in .NET 7 and earlier) iterating the child list in reverse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this relate to ".NET 7"? Is this actually about the version of Roslyn that's being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this comment is about the version of Roslyn that's used.
Perhaps this should be: "Avoid performance issue in earlier implementations of ChildSyntaxList iterating the child list in reverse ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. You could just be explicit and include a link to the Roslyn PR that fixed it. Essentially we're saying if you're running with an earlier Roslyn than that, you could have perf issues. I'm just questioning the original wording because you will likely end up getting that fix as part of .NET 7 SDK updates and as part of VS updates, right?
/backport to release/7.0 |
/backport to release/6.0 |
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4483155231 |
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/4483155981 |
See dotnet/roslyn#66475