Skip to content

Adjust caching logic when a NodeStateTable is empty#62068

Merged
jcouv merged 5 commits intodotnet:mainfrom
jcouv:sg-crash
Jun 29, 2022
Merged

Adjust caching logic when a NodeStateTable is empty#62068
jcouv merged 5 commits intodotnet:mainfrom
jcouv:sg-crash

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv jcouv commented Jun 21, 2022

Fixes #57455

For context, here's the relevant logic in BatchNode. What happened was that sourceTable would be empty (the new source removed the attribute, so no syntax is returned from this source), but sourceTable.IsCached would return true which leads us to use the old entries newTable.TryUseCachedEntries(stopwatch.Elapsed, sourceInputs).

        public NodeStateTable<ImmutableArray<TInput>> UpdateStateTable(DriverStateTable.Builder builder, NodeStateTable<ImmutableArray<TInput>> previousTable, CancellationToken cancellationToken)
        {
            // grab the source inputs
            var sourceTable = builder.GetLatestStateTableForNode(_sourceNode);

            // Semantics of a batch transform:
            // Batches will always exist (a batch of the empty table is still [])
            // There is only ever one input, the batch of the upstream table
            // - Output is cached when upstream is all cached
            // - Added when the previous table was empty
            // - Modified otherwise

            // update the table
            var newTable = builder.CreateTableBuilder(previousTable, _name);

            // If this execution is tracking steps, then the source table should have also tracked steps or be the empty table.
            Debug.Assert(!newTable.TrackIncrementalSteps || (sourceTable.HasTrackedSteps || sourceTable.IsEmpty));

            var stopwatch = SharedStopwatch.StartNew();

            var batchedSourceEntries = sourceTable.Batch();
            var sourceValues = batchedSourceEntries.SelectAsArray(sourceEntry => sourceEntry.State != EntryState.Removed, sourceEntry => sourceEntry.Item);
            var sourceInputs = newTable.TrackIncrementalSteps ? batchedSourceEntries.SelectAsArray(sourceEntry => (sourceEntry.Step!, sourceEntry.OutputIndex)) : default;

            if (previousTable.IsEmpty)
            {
                newTable.AddEntry(sourceValues, EntryState.Added, stopwatch.Elapsed, sourceInputs, EntryState.Added);
            }
            else if (!sourceTable.IsCached || !newTable.TryUseCachedEntries(stopwatch.Elapsed, sourceInputs))
            {
                if (!newTable.TryModifyEntry(sourceValues, _comparer, stopwatch.Elapsed, sourceInputs, EntryState.Modified))
                {
                    newTable.AddEntry(sourceValues, EntryState.Added, stopwatch.Elapsed, sourceInputs, EntryState.Added);
                }
            }

            return newTable.ToImmutableAndFree();
        }

@jcouv jcouv self-assigned this Jun 21, 2022
@ghost ghost added the Area-Compilers label Jun 21, 2022
@jcouv jcouv marked this pull request as ready for review June 22, 2022 17:31
@jcouv jcouv requested a review from a team as a code owner June 22, 2022 17:31
@jcouv jcouv requested a review from chsienki June 22, 2022 17:31
@jcouv jcouv added the Feature - Source Generators Source Generators label Jun 22, 2022
@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Jun 22, 2022

@chsienki @dotnet/roslyn-compiler for review. Thanks

Copy link
Copy Markdown
Member

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Jun 22, 2022

@cston @dotnet/roslyn-compiler for second review. Thanks

ctx.RegisterSourceOutput(compilationAndClasses, (context, ct) => validate(ct.Item1, ct.Item2));
}));

// run the generator once, and check it was passed the parse options
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and check it was passed the parse options

Was this checked? It's not clear that it needs to be checked, but if it's not, consider removing the comment.

}

[Fact, WorkItem(57455, "https://github.com/dotnet/roslyn/issues/57455")]
public void RemoveTriggeringSyntaxAndVerifySyntaxTreeConsistentWithCompilation()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

VerifySyntaxTreeConsistentWithCompilation

Are we verifying the syntax tree below, or is this just implicit in the test completing without exceptions?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We are verifying the syntax tree given to the SG comes from the correct compilation. So I would say "both".

@jcouv jcouv enabled auto-merge (squash) June 28, 2022 23:26
@jcouv jcouv merged commit 8ef429e into dotnet:main Jun 29, 2022
@ghost ghost added this to the Next milestone Jun 29, 2022
@jcouv jcouv deleted the sg-crash branch June 29, 2022 18:29
jcouv added a commit to jcouv/roslyn that referenced this pull request Jun 29, 2022
@allisonchou allisonchou modified the milestones: Next, 17.4 P1 Jul 26, 2022
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.

JsonSourceGenerator throwing exceptions after edit removing attribute

4 participants