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.

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

Refactor ISyntaxInputNodes #58800

Merged
merged 24 commits into from
Feb 2, 2022
Merged

Refactor ISyntaxInputNodes #58800

merged 24 commits into from
Feb 2, 2022

Conversation

chsienki
Copy link
Member

@chsienki chsienki commented Jan 12, 2022 •
edited
Loading

Refactors syntax selection logic in the generator driver, so that the driver itself doesn't have to special case syntax nodes:

  • Create SyntaxStore which handles all syntax related options.
  • Factor out state table logic into its own class
  • Unify SyntaxInputNodes into a single class
    • Add 'Strategies' than can be passes to the input node to perform lookup differently as needed

Note this is mostly a mechanical refactoring, so it may be easiest to go commit-by-commit. There are a couple of file renames that don't happen until the last - 1 commit, so if not doing one-by-one I suggest excluding that commit at least to make it easier to follow.

@runfoapp runfoapp bot mentioned this pull request Jan 12, 2022
Closed
@chsienki chsienki added the Feature - Source Generators Source Generators label Jan 13, 2022
@chsienki chsienki marked this pull request as ready for review January 13, 2022 19:32
@chsienki chsienki requested a review from a team as a code owner January 13, 2022 19:32
@chsienki
Copy link
Member Author

@dotnet/roslyn-compiler for review please.

@jaredpar
Copy link
Member

@cston, @RikkiGibson PTAL

cston
cston reviewed Jan 14, 2022
View reviewed changes
src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs
@@ -269,7 +271,7 @@ internal GeneratorDriverState RunGeneratorsCore(Compilation compilation, Diagnos
}
}

state = state.With(stateTable: driverStateBuilder.ToImmutable(), generatorStates: stateBuilder.ToImmutableAndFree(), runTime: timer.Elapsed);
state = state.With(stateTable: driverStateBuilder.ToImmutable(), syntaxStore: syntaxStoreBuilder.ToImmutable(), generatorStates: stateBuilder.ToImmutableAndFree(), runTime: timer.Elapsed);
Copy link
Member

@cston cston Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We have two distinct instances of SyntaxStore - from driverStateBuilder.ToImmutable().SyntaxStore and from syntaxStoreBuilder.ToImmutable(). If it's necessary to pass a SyntaxStore instance directly to With(), consider refactoring to:

Suggested change
state = state.With(stateTable: driverStateBuilder.ToImmutable(), syntaxStore: syntaxStoreBuilder.ToImmutable(), generatorStates: stateBuilder.ToImmutableAndFree(), runTime: timer.Elapsed);
var stateTable = driverStateBuilder.ToImmutable();
state = state.With(stateTable, stateTable.SyntaxStore, generatorStates: stateBuilder.ToImmutableAndFree(), runTime: timer.Elapsed);
``` #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

DriverStateTable doesnt actually hold onto the SyntaxStore so we only have the single instance.

@jaredpar
Copy link
Member

PR looks like it's getting stale? Is something hloding it up?

@chsienki
Copy link
Member Author

@cston and @RikkiGibson for a second round of reviews please :)

cston
cston reviewed Jan 29, 2022
View reviewed changes
src/Compilers/Core/Portable/SourceGeneration/SyntaxStore.cs Outdated
_compilation = compilation;
}

public Builder ToBuilder(Compilation compilation, ImmutableArray<SyntaxInputNode> syntaxInputNodes, bool enableTracking, CancellationToken cancellationToken = default) => new Builder(compilation, syntaxInputNodes, enableTracking, this, cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

cancellationToken = default

It looks like there is only one caller of ToBuilder(). Given that, consider making cancellationToken not optional.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's also used in a test, but I'll make it required and just pass it in explicitly there.

cston
cston reviewed Jan 29, 2022
View reviewed changes
src/Compilers/Core/Portable/SourceGeneration/SyntaxStore.cs Outdated
private readonly SyntaxStore _previous;
private readonly CancellationToken _cancellationToken;

internal Builder(Compilation compilation, ImmutableArray<SyntaxInputNode> syntaxInputNodes, bool enableTracking, SyntaxStore previousStore, CancellationToken cancellationToken = default)
Copy link
Member

Choose a reason for hiding this comment

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

= default

cancellationToken does not need to be optional.

@RikkiGibson RikkiGibson self-assigned this Jan 29, 2022
RikkiGibson
RikkiGibson approved these changes Jan 31, 2022
View reviewed changes
Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM with a few comments/nits

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriverState.cs
@@ -95,6 +99,7 @@ namespace Microsoft.CodeAnalysis
ImmutableArray<GeneratorState>? generatorStates = null,
ImmutableArray<AdditionalText>? additionalTexts = null,
DriverStateTable? stateTable = null,
SyntaxStore? syntaxStore = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if this and 'GeneratorState' were records. I don't think we've had many opportunities to dogfood records.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I think in the distant past there was even a PROTOTYPE comment about doing that, but the features went in side by side and we didn't get a chance to switch over. I've opened #59199 to track doing that, but I'll keep it separate from this PR to reduce churn in an already churn-y PR.

src/Compilers/Core/Portable/SourceGeneration/Nodes/PredicateSyntaxStrategy.cs

namespace Microsoft.CodeAnalysis
{
internal sealed class PredicateSyntaxStrategy<T> : ISyntaxSelectionStrategy<T>
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if there's a more meaningful type parameter name that could be used, possibly throughout this layer. Mostly the type parameter is for the output type of a single transform in the generator, right?

src/Compilers/Core/Portable/SourceGeneration/SyntaxStore.cs
// update each tree for the builders, sharing the semantic model
foreach ((var tree, var state, var syntaxTreeIndex, var stepInfo) in syntaxTreeState)
{
var root = new Lazy<SyntaxNode>(() => tree.GetRoot(_cancellationToken));
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't related to your changes, but it seems like it would be simpler (fewer allocations) if we simply passed the 'tree' instead of 'root' to the builder here and have the builder call GetRoot. If a syntax tree is doing a parse in response to a GetRoot call it is usually caching the root afterwards anyway. Right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It probably would, but its hard to be sure. In the case of a recoverable syntax tree the root is held via WeakReference, so it might stick around for the next generator, but under high memory pressure it also might get swapped out before the next walker gets to it. We should probably profile this before making any changes to it.

chsienki added 22 commits February 2, 2022 12:08
@chsienki
Move syntax state tables into SyntaxStore
eaa6865
@chsienki
Remove unused parameter
41f45f7
@chsienki
Pass in tracking bool rather than entire driver state
e0df5c2
@chsienki
Create and store the syntaxstore outside of the driver state table
dda84ce
@chsienki
Cleanup comments
f81c3bb
@chsienki
Remove GetSyntaxInputStore from driverstatetable.builder
45eaacc
@chsienki
Calculate isCompilationCached internally
79b82ce
@chsienki
Remove unused check + state save
e0e90c9
@chsienki
Remove unused synatx store from builder
d78f07c
@chsienki
Intermediate refactor:
60e0bd9 
create a wrapper ISynaxNode2 that splits out the incremental node from the syntax node interfaces.
@chsienki
Continue untangling syntax node from input nodes
88a0ac3
@chsienki
Usings
6515757
@chsienki
Clean up parent reference
949b0d9
@chsienki
Rename
6613ac2
@chsienki
Rename files
f089307
@chsienki
Fix Formatting
fe6b6c1
@chsienki
WIP
edf1020
@chsienki
PR Feedback
bd3114f
@chsienki
Restore global.json
050de41
@chsienki
Feedback: readonly, require token.
a675d11
@chsienki
PR Feedback
0029fab
@chsienki
Fixup after re-base
45e4e23
@chsienki chsienki force-pushed the source-generators/syntax-store branch from 2e2dee4 to 45e4e23 Compare February 2, 2022 20:23
@chsienki
Copy link
Member Author

chsienki commented Feb 2, 2022

Rebased onto latest main.

@chsienki chsienki merged commit a7e3c09 into dotnet:main Feb 2, 2022
@ghost ghost added this to the Next milestone Feb 2, 2022
@RikkiGibson RikkiGibson mentioned this pull request Feb 4, 2022
Closed
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@cston cston cston approved these changes

Assignees

@RikkiGibson RikkiGibson

Labels
Area-Compilers Feature - Source Generators Source Generators
Projects
None yet
Milestone
17.2.P1
Development

Successfully merging this pull request may close these issues.
4 participants
@chsienki @jaredpar @RikkiGibson @cston