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

Incremental Generator Work Tracking API Implementation #55469

Merged
merged 23 commits into from
Dec 7, 2021
Merged

Incremental Generator Work Tracking API Implementation #55469

merged 23 commits into from
Dec 7, 2021

Conversation

jkoritzinsky
Copy link
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
Member Author

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
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
Member

@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
Member Author

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
src/Compilers/Core/Portable/SourceGeneration/Nodes/SyntaxReceiverInputNode.cs
@@ -65,20 +76,30 @@ public Builder(SyntaxReceiverInputNode owner, DriverStateTable driverStateTable)

public ISyntaxInputNode SyntaxInputNode { get => _owner; }

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

@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
Member Author

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
Member Author

@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
Member Author

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

@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

@cston cston cston approved these changes

@sharwell sharwell Awaiting requested review from sharwell

Assignees

@chsienki chsienki

Labels
Area-Compilers New 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