Refactor ISyntaxInputNodes#58800
Conversation
|
@dotnet/roslyn-compiler for review please. |
|
@cston, @RikkiGibson PTAL |
| } | ||
|
|
||
| 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); |
There was a problem hiding this comment.
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:
| 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 |
There was a problem hiding this comment.
DriverStateTable doesnt actually hold onto the SyntaxStore so we only have the single instance.
|
PR looks like it's getting stale? Is something hloding it up? |
|
@cston and @RikkiGibson for a second round of reviews please :) |
| _compilation = compilation; | ||
| } | ||
|
|
||
| public Builder ToBuilder(Compilation compilation, ImmutableArray<SyntaxInputNode> syntaxInputNodes, bool enableTracking, CancellationToken cancellationToken = default) => new Builder(compilation, syntaxInputNodes, enableTracking, this, cancellationToken); |
There was a problem hiding this comment.
It's also used in a test, but I'll make it required and just pass it in explicitly there.
| private readonly SyntaxStore _previous; | ||
| private readonly CancellationToken _cancellationToken; | ||
|
|
||
| internal Builder(Compilation compilation, ImmutableArray<SyntaxInputNode> syntaxInputNodes, bool enableTracking, SyntaxStore previousStore, CancellationToken cancellationToken = default) |
RikkiGibson
left a comment
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM with a few comments/nits
| ImmutableArray<GeneratorState>? generatorStates = null, | ||
| ImmutableArray<AdditionalText>? additionalTexts = null, | ||
| DriverStateTable? stateTable = null, | ||
| SyntaxStore? syntaxStore = null, |
There was a problem hiding this comment.
It would be nice if this and 'GeneratorState' were records. I don't think we've had many opportunities to dogfood records.
There was a problem hiding this comment.
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.
|
|
||
| namespace Microsoft.CodeAnalysis | ||
| { | ||
| internal sealed class PredicateSyntaxStrategy<T> : ISyntaxSelectionStrategy<T> |
There was a problem hiding this comment.
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?
| // 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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
create a wrapper ISynaxNode2 that splits out the incremental node from the syntax node interfaces.
chsienki
force-pushed
the
source-generators/syntax-store
branch
from
2e2dee4 to
45e4e23
Compare
|
Rebased onto latest main. |
ghost
added this to the
4 participants
Refactors syntax selection logic in the generator driver, so that the driver itself doesn't have to special case syntax nodes:
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.