Replace SyntaxTree caching with IDE document state preservation#50490
Replace SyntaxTree caching with IDE document state preservation#5049016 commits merged intodotnet:mainfrom
Conversation
d168f1e to
99403fc
Compare
| SourceText text, | ||
| CSharpParseOptions? options = null, | ||
| string path = "", | ||
| CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Being internal, consider making all the args required?
| SolutionState solution, | ||
| Compilation compilation, | ||
| ImmutableArray<SourceGeneratedDocumentState>? authoritativeGeneratedDocuments, | ||
| ImmutableArray<SourceGeneratedDocumentState> nonAuthoritativeGeneratedDocuments, |
There was a problem hiding this comment.
Need @jasonmalinowski 's views on this. I forget what FinalizeCompilation means. I thought it had meant: move the compilation to the final state. Which means it seems odd to be using incorrect states here. Is that not the case? Is FinalizeCompilation for a different purpose?
There was a problem hiding this comment.
So yes, FinalizeCompilation is the final (hah!) called when we're producing a compilation. I think what might address your concern is if instead of nonAuthoritativeGeneratedDocuments it was called priorGeneratedDocumentsThatMayBeReused or something along those lines to clarify that:
- These are from a prior run, and may be stale/old.
- It's not required that they are actually consumed.
|
@jasonmalinowski can you PTAL as this hits generators, compilation trackers, and is a big net perf win for an important scenario. |
| // We can reuse the previous TextAndVersion if the checksum from the text matches. | ||
| if (previousState is null | ||
| || !previousState.TryGetTextAndVersion(out var textAndVersion) | ||
| || Checksum.From(textAndVersion.Text.GetChecksum()) != Checksum.From(generatedSourceText.GetChecksum())) |
There was a problem hiding this comment.
nit: cnosider just a function that compares the results of GetChecksum so you don't have to wrap in another Checksum object.
| // recoverable text but not a new tree, it would mean tree.GetText() could still potentially return | ||
| // the non-recoverable text, but asking the document directly for it's text would give a recoverable | ||
| // text with a different object identity. We only need to create a recoverable text if the source | ||
| // changed; otherwise, the reused text source would be recoverable. |
There was a problem hiding this comment.
wow. do we have a test to validate this :)
There was a problem hiding this comment.
I don't know if it's even possible to validate this.
There was a problem hiding this comment.
In the Workspace tests we have helpers that create workspace instances both with and without recoverable trees, so you have plenty of tools at your disposal.
(That said I think a greater simplification here can eliminate a bunch of this...)
There was a problem hiding this comment.
If this is staying around, consider adding an assert that CreateRecoverableText isn't called with a recoverable text to start with? or if it is, just make it no-op?
- Don't take a cancellation token, since the whole point is not to do expensive work - Just keep fields and parameters in a consistent order
22c4b27 to
4ecdc8c
Compare
jasonmalinowski
left a comment
There was a problem hiding this comment.
So I've simplified the Create() method and added some tests, which did catch one issue of this not working in the mainline path. So I think this is now good to take, but somebody else shoudl review my work or otherwise this is somewhat cheating.
| private readonly ImmutableDictionary<string, ReportDiagnostic> _diagnosticOptions; | ||
| private CSharpSyntaxNode? _lazyRoot; | ||
|
|
||
| internal LazySyntaxTree( |
There was a problem hiding this comment.
I'm somewhat on the fence about introducing a new type of SyntaxTree but I agree it makes sense given the API we have today.
I actually think we should avoid returning realized SyntaxTree instances from the generator at all, and instead have a method that does it lazily. Given that that is a breaking change however, lets punt it down the road, and take this approach for now.
There was a problem hiding this comment.
Agreed; I think this is the best approach for now, and we can consider using the next major version to just break the API.
|
|
||
| _text = text; | ||
| _options = options; | ||
| _path = path ?? string.Empty; |
There was a problem hiding this comment.
path [](start = 24, length = 4)
path is declared as not null.
There was a problem hiding this comment.
So for regular trees this is what we're doing as well, which seems odd. We always silently converted a null going in to an empty string. When we annotated the methods we then kept them as non-nullable to discourage people from passing null (since it's probably not entirely what they wanted....) but it was still allowed and silently changed.
This is matching the other types. I can change this, but it'll be inconsistent unless I change the other types as well. Thoughts?
There was a problem hiding this comment.
Let's keep it as is, and consistent. Thanks for the explanation.
In reply to: 601792055 [](ancestors = 601792055)
Supersedes #50171
Fixes AB#1254804
Create a public API for syntax tree cachingMove caching to the Workspaces/Host (IDE scenarios), and disable caching for command line buildsSyntaxTreefrom source generatorsGeneratorDriverto return lazySyntaxTreeinstances instead of parsing them eagerly