Skip to content

SyntaxValueProvider: avoid performance issue with syntax list containing many items #83483

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

Merged
cston merged 3 commits into from
Mar 21, 2023
Merged

SyntaxValueProvider: avoid performance issue with syntax list containing many items #83483

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7083bc
Source generator: avoid performance issue with syntax list containing…
cston Mar 15, 2023
f8aef57
Iterate through child list forward first
cston Mar 15, 2023
05be676
Update comment
cston Mar 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Iterate through child list forward first
  • Loading branch information
@cston
cston committed Mar 15, 2023
commit f8aef57b88c3b94b3c452db657a3efad2da7b4bc
10 changes: 9 additions & 1 deletion src/libraries/Common/src/Roslyn/SyntaxValueProvider_ForAttributeWithSimpleName.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,15 @@ void processMember(
// For any other node, just keep recursing deeper to see if we can find an attribute. Note: we cannot
// terminate the search anywhere as attributes may be found on things like local functions, and that
// means having to dive deep into statements and expressions.
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
Copy link
Member

@stephentoub stephentoub Mar 16, 2023

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?

Copy link
Contributor Author

@cston cston Mar 16, 2023 •
edited
Loading

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

Copy link
Member

@stephentoub stephentoub Mar 16, 2023

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?

// by iterating forward first, to populate the cache.
foreach (var childNode in childNodesAndTokens)
{
}

foreach (var child in childNodesAndTokens.Reverse())
{
if (child.IsNode)
nodeStack.Append(child.AsNode()!);
Expand Down