Hold onto and reuse GeneratorDrivers #53709
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
This was null-returning originally because generators were a preview feature and we didn't want to create GeneratorDrivers when the preview feature was disabled. Then we used kept it when C# had generators but not VB. At this point any language that creates Compilations always creates generators, so we can simplify.
We're going to need to be managing instances more carefully, so keep a 1:1 mapping between them. A ConditionalWeakTable here isn't the best way to be doing this, since we own the type that's the key, but our inheritance structure is a bit odd: TextDocumentState is used as a base type for some things (like regular files) but is also used directly as a type for additional files. We really should introduce a new derived type to directly represent additional files, but that's a more invasive change.
Right now we were simply creating a new GeneratorDriver every time we needed to run generators; now that the compiler supports an incremental API we need to hold onto the GeneratorDriver between runs so it can cache inputs smartly. For the most part the change is simple: we simply hold onto the GeneratorDriver once we're done running generators, and just give it a new compilation when we want to incrementally run. The only real "tricky" bit is there are some pieces of state that aren't held by the compiler, but rather by the GeneratorDriver itself, namely: 1. The list of generators to themselves. 2. The list of additional files -- that's simply not held by a Compilation at all. 3. The parse options that's used for parsing generated files -- you can't get ParseOptions from the CompilationOptions. 4. The parsed .editorconfigs. These are precisely the same pieces of state that are passed to GeneratorDriver.Create when we initially create one. For these pieces of state, if they're changing we need to also update the GeneratorDriver itself with the APIs that exist on GeneratorDriver along when we incrementally update a Compilation. To do this we transform the GeneratorDriver the same way we do a Compilation through in-progress states. Currently, there's some APIs missing on the GeneratorDriver API, so sometimes we just drop the driver entirely which will force a rerun from scratch after those happen.
jasonmalinowski
force-pushed
the
cache-generator-drivers-between-runs
branch
from
2be334e
to
3632b0c
Compare
jasonmalinowski
commented
Jun 9, 2021
...e/Workspace/Solution/SolutionState.CompilationAndGeneratorDriverTranslationAction_Actions.cs
Show resolved
Hide resolved
jasonmalinowski
commented
Jun 9, 2021
...e/Workspace/Solution/SolutionState.CompilationAndGeneratorDriverTranslationAction_Actions.cs
Show resolved
Hide resolved
jasonmalinowski
commented
Jun 9, 2021
...e/Workspace/Solution/SolutionState.CompilationAndGeneratorDriverTranslationAction_Actions.cs
Show resolved
Hide resolved
jasonmalinowski
commented
Jun 9, 2021
| @@ -14,11 +15,18 @@ namespace Microsoft.CodeAnalysis.Diagnostics | |||
| internal sealed class AdditionalTextWithState : AdditionalText | |||
| { | |||
| private readonly TextDocumentState _documentState; | |||
| private static readonly ConditionalWeakTable<TextDocumentState, AdditionalText> _additionalTextsForDocumentStates = new(); | |||
There was a problem hiding this comment.
See individual commit comment on this -- this is a bit of a hack to keep this PR size down; I suspect the alternate approach will do more churn than the rest of this PR, so doing one at a time may be best.
There was a problem hiding this comment.
Looks like my hunch about churn may have not been as bad as I thought, but you're on the right track with the other PR I think.
sharwell
approved these changes
Jun 14, 2021
jasonmalinowski
commented
Jun 14, 2021
| // The other one should have been kept constant because that didn't change. | ||
| Assert.Equal(3, generator.AdditionalFilesConvertedCount); | ||
|
|
||
| // Change one of the source documents, and rerun; we should again only reprocess that one change. |
There was a problem hiding this comment.
Comment seems inaccurate, since this change will not result in anything reprocessed (the comment on line 101 seems correct)
There was a problem hiding this comment.
The generator driver reprocesses the change. The source generator does no work as part of handling this change. I can reword but I'm not sure it's confusing in context of the assertions.
chsienki
approved these changes
Jun 15, 2021
|
@chsienki The squash merge is making the follow-up PR very difficult to rebase. In the future, please use a normal merge commit unless verified with the author and other contributor(s) of a pull request. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now we were simply creating a new GeneratorDriver every time we needed to run generators; this will allow us to more efficiently reuse prior runs with our new incremental API.