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

Lazily evaluate the root node for syntax trees #55716

Merged
merged 1 commit into from
Aug 27, 2021

Conversation

chsienki
Copy link
Contributor

This avoids an issue where we continually recover syntax trees in the IDE, even when they havn't changed, which has a noticeable perf impact.

@chsienki chsienki requested a review from a team as a code owner August 19, 2021 00:12
@chsienki chsienki added this to the 17.0 milestone Aug 19, 2021
@chsienki
Copy link
Contributor Author

@dotnet/roslyn-compiler for review please.

@@ -16,7 +17,7 @@ internal interface ISyntaxInputBuilder
{
ISyntaxInputNode SyntaxInputNode { get; }

void VisitTree(SyntaxNode root, EntryState state, SemanticModel? model, CancellationToken cancellationToken);
void VisitTree(Lazy<SyntaxNode> root, EntryState state, SemanticModel? model, CancellationToken cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

❔ Can we not make this accept SyntaxTree instead? The call to GetRoot is cheap at the point where it's required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it cheap to call it multiple times? If you have multiple generators, each one will call tree.GetRoot() I didn't want to asssume that subsequent calls would be cheaper...

Copy link
Member

Choose a reason for hiding this comment

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

It's unlikely for the call to GetRoot to be expensive for the second and subsequent calls (when needed). There is a brief moment where the root could be collected by the GC when moving from one source generator to the next. It could be prevented like this:

Suggested change
void VisitTree(Lazy<SyntaxNode> root, EntryState state, SemanticModel? model, CancellationToken cancellationToken);
void VisitTree(SyntaxTree tree, ref SyntaxNode? lazyRoot, EntryState state, SemanticModel? model, CancellationToken cancellationToken);

Then where you need the tree:

var root = LazyInitialization.EnsureInitialized(
    ref lazyRoot,
    static arg => arg.tree.GetRoot(arg.cancellationToken),
    (tree, cancellationToken));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, Jared pointed out that if we only get the root when the tree is different, then it's unlikely to be a recoverable tree in the first place, in which case GetRoot should be cheap. @sharwell does that seem correct?

Copy link
Member

@sharwell sharwell Aug 19, 2021

Choose a reason for hiding this comment

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

I'm not sure why that would make a difference. It should be cheap, even if recoverable, but either way you can eliminate the use of Lazy<T> with the pattern I gave.

@chsienki
Copy link
Contributor Author

@dotnet/roslyn-compiler for a second review please :)

@chsienki chsienki enabled auto-merge (squash) August 25, 2021 20:47
@chsienki
Copy link
Contributor Author

Rebasing to main for merge.

@chsienki chsienki force-pushed the source-generators/lazy-syntax-tree branch from 0435497 to dc5397a Compare August 26, 2021 22:38
@chsienki chsienki merged commit 6c9697c into dotnet:main Aug 27, 2021
@ghost ghost modified the milestones: 17.0, Next Aug 27, 2021
@dibarbet dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants