Skip to content

Incremental Generator Work Tracking API Implementation#55469

Merged
jkoritzinsky merged 23 commits intodotnet:mainfrom
jkoritzinsky:incremental-generator-work-tracking
Dec 7, 2021
Merged

Incremental Generator Work Tracking API Implementation#55469
jkoritzinsky merged 23 commits intodotnet:mainfrom
jkoritzinsky:incremental-generator-work-tracking

Conversation

@jkoritzinsky
Copy link
Copy Markdown
Member

@jkoritzinsky jkoritzinsky commented Aug 6, 2021 •
edited
Loading

Implement incremental generator work tracking API proposed in #54832 and update incremental generator API tests to use it to validate incrementality.

Fixes #54832

@jkoritzinsky

This comment has been minimized.

Sign in to view
@jkoritzinsky jkoritzinsky force-pushed the incremental-generator-work-tracking branch from 8059b7c to 4891c50 Compare September 9, 2021 17:31
@jkoritzinsky jkoritzinsky changed the base branch from main to release/dev17.0 September 9, 2021 17:31
@jkoritzinsky jkoritzinsky marked this pull request as ready for review September 10, 2021 16:48
@jkoritzinsky jkoritzinsky requested review from a team as code owners September 10, 2021 16:48
@jkoritzinsky
Copy link
Copy Markdown
Member Author

jkoritzinsky commented Sep 10, 2021

This PR is now ready for review now that the API shape has been approved

cc: @chsienki @sharwell

@jkoritzinsky jkoritzinsky added this to the 17.0 milestone Sep 10, 2021
@jkoritzinsky jkoritzinsky force-pushed the incremental-generator-work-tracking branch from f1da0a8 to 93292b6 Compare September 15, 2021 17:50
@jkoritzinsky jkoritzinsky changed the base branch from release/dev17.0 to main September 15, 2021 17:54
@jkoritzinsky jkoritzinsky force-pushed the incremental-generator-work-tracking branch 2 times, most recently from bccf607 to 4fb1289 Compare September 15, 2021 18:47
@jkoritzinsky jkoritzinsky force-pushed the incremental-generator-work-tracking branch from 4fb1289 to d1be32b Compare September 15, 2021 20:06
@chsienki chsienki self-assigned this Sep 16, 2021
chsienki
chsienki reviewed Sep 16, 2021
View reviewed changes
chsienki
chsienki reviewed Sep 16, 2021
View reviewed changes
chsienki
chsienki reviewed Sep 16, 2021
View reviewed changes
chsienki
chsienki reviewed Sep 16, 2021
View reviewed changes
chsienki
chsienki reviewed Sep 16, 2021
View reviewed changes
chsienki
chsienki reviewed Sep 16, 2021
View reviewed changes
cston
cston reviewed Nov 29, 2021
View reviewed changes
cston
cston reviewed Nov 29, 2021
View reviewed changes
src/Compilers/Core/Portable/SourceGeneration/Nodes/SyntaxInputNode.cs
_transformTable.RemoveEntries();
_filterTable.TryRemoveEntries(TimeSpan.Zero, ImmutableArray<(IncrementalGeneratorRunStep, int)>.Empty, out ImmutableArray<SyntaxNode> removedNodes);

for (int i = 0; i < removedNodes.Length; i++)
Copy link
Copy Markdown
Contributor

@cston cston Nov 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

for (int i = 0; i < removedNodes.Length; i++)

I'm confused by this loop. Are we creating more removed entries than before this change? #Closed

Copy link
Copy Markdown
Member Author

@jkoritzinsky jkoritzinsky Nov 30, 2021

Choose a reason for hiding this comment

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

Yes, beforehand we did not accurately create enough "Removed" entries in the table, but since entries with the Removed state were ignored in later stages anyway (only kept in very specific scenarios), the bug was unnoticed.

cston
cston reviewed Nov 30, 2021
View reviewed changes
cston
cston reviewed Nov 30, 2021
View reviewed changes
src/Compilers/Core/Portable/SourceGeneration/Nodes/SyntaxReceiverInputNode.cs
public ISyntaxInputNode SyntaxInputNode { get => _owner; }

[MemberNotNullWhen(true, nameof(_inputSteps))]
private bool TrackIncrementalSteps => _nodeStateTable.TrackIncrementalSteps;
Copy link
Copy Markdown
Contributor

@cston cston Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

_nodeStateTable.TrackIncrementalSteps

If TrackIncrementalSteps is tied to _nodeStateTable.TrackIncrementalSteps, did we need the trackIncrementalSteps parameter in the constructor? #Closed

Copy link
Copy Markdown
Member Author

@jkoritzinsky jkoritzinsky Nov 30, 2021

Choose a reason for hiding this comment

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

We produce the value for _nodeStateTable in the constructor based partially on the trackIncrementalSteps parameter in the constructor.

@jkoritzinsky
Copy link
Copy Markdown
Member Author

jkoritzinsky commented Dec 3, 2021

@chsienki I had to make some modifications to the tests to react to the changes in #57888 in case you're interested in taking a look at the test updates.

@jkoritzinsky
Copy link
Copy Markdown
Member Author

jkoritzinsky commented Dec 6, 2021

@cston I've addressed your feedback and got CI on this PR back to green. Do you have any more feedback?

cston
cston reviewed Dec 6, 2021
View reviewed changes
@jkoritzinsky jkoritzinsky disabled auto-merge December 7, 2021 05:08
@jkoritzinsky jkoritzinsky enabled auto-merge (squash) December 7, 2021 05:08
@jkoritzinsky jkoritzinsky merged commit 95809b0 into dotnet:main Dec 7, 2021
@ghost ghost modified the milestones: 17.1, Next Dec 7, 2021
@Cosifne Cosifne modified the milestones: Next, 17.1.P3 Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chsienki chsienki chsienki approved these changes

@sharwell sharwell Awaiting requested review from sharwell

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@chsienki chsienki

Labels

Area-Compilers Feature - Source Generators Source Generators

Projects

None yet

Milestone

17.1.P3

Development

Successfully merging this pull request may close these issues.

Incremental Generator Work Tracking APIs

6 participants

@jkoritzinsky @RikkiGibson @jcouv @chsienki @cston @Cosifne