Simplify how frozen partial documents are produced. #72106
Conversation
ghost
added
Area-IDE
untriaged
CyrusNajmabadi
force-pushed
the
frozenPartialOuter
branch
from
687abc9 to
90d4413
Compare
| // to contribute any changes that would affect which types we think are designable), and we want to be | ||
| // very fast to update the ui as a user types. | ||
| var frozenSolution = await solution.WithFrozenPartialCompilationsAsync(cancellationToken).ConfigureAwait(false); | ||
| var frozenSolution = solution.WithFrozenPartialCompilations(cancellationToken); |
There was a problem hiding this comment.
became synchronous as this can be done entirely without async work.
There was a problem hiding this comment.
It looks like it was never async before, other than because of us calling the AsyncLazy GetValueAsync? I wonder if it's good/bad for us to be using AsyncLazy too for non-async cases when we want regular Lazy but also have a bit friendlier of a cancellation story.
There was a problem hiding this comment.
yeah. it's def interesting. i don't personally mind it :)
| { | ||
| var state = this.ReadState(); | ||
|
|
||
| // If we're already finalized then just return what we have, and with the frozen bit flipped so that |
There was a problem hiding this comment.
all this work got effectively inlined into this method.
There was a problem hiding this comment.
the diff is awful. i recommend not looking at it inlined.
There was a problem hiding this comment.
ok. because the diff was so awful, i moved this method much lower. so it just shows as a full add.
| // incorporate the new document. | ||
| // If we're finalized and already frozen, we can just use ourselves. | ||
| if (state is FinalCompilationTrackerState { IsFrozen: true } finalState) | ||
| return this; |
There was a problem hiding this comment.
initial case. if we have nothing to do, do nothing :)
ughhhhhhhh |
|
@jasonmalinowski this is ready for another pass. |
| // Because we froze before ever even looking at anything semantics related, we should have no documents in | ||
| // this project. | ||
| var frozenSolution = await project.Solution.WithFrozenPartialCompilationsAsync(CancellationToken.None); | ||
| var frozenSolution = project.Solution.WithFrozenPartialCompilations(CancellationToken.None); |
There was a problem hiding this comment.
yes. the sync one is preferred. if there is still an async one, i intend to remove it (can be done in followup). the general principle we want to follow is that this is available and synchronous for code that is intended ot be in low latency features, where they don't want to even do any work on a threadpool if avoidable.
| _cachedFrozenSolution = cachedFrozenSolution ?? new AsyncLazy<Solution>( | ||
| c => Task.FromResult(ComputeFrozenSolution(c)), | ||
| c => ComputeFrozenSolution(c)); |
There was a problem hiding this comment.
Can this use your other helper? If this is coming in a queued PR then feel free to resolve this comment.
| private ConditionalWeakTable<ISymbol, ProjectId?>? _unrootedSymbolToProjectId; | ||
| private static readonly Func<ConditionalWeakTable<ISymbol, ProjectId?>> s_createTable = () => new ConditionalWeakTable<ISymbol, ProjectId?>(); | ||
|
|
||
| private readonly AsyncLazy<SolutionCompilationState> _cachedFrozenSnapshot; |
There was a problem hiding this comment.
Isn't this just undoing #72193? Becuase I liked that one. I didn't undestand why we had AsyncLazies at both levels.
There was a problem hiding this comment.
it's not quite undoing it. the cachgin, and what we hand out, is still cached Solution instances (which is good for downstream consumers). However, under the covers there are a couple of paths that get to asking for the frozen compilation state. And this ensures they can happen and still get the same result. this is not really that necessary though, and i will considering removing it. i just very much liked the idea that sharing woudl happen if possible.
4 participants
Followup to #72090
The core idea here relates to how we get a frozen partial solution snapshot that contains a document within it.
The prior way of doing this was to do the freezing step, passing the docstate inwards for the document we wanted in the final frozen solution. This required deep knowledge within CompilationTrackers for how to freeze, but also contain a document.
The new logic inverts this idea. Now, we simply freeze the solution. Then we take that frozen solution, and we ensure that it contains the doc-state for the document in question (adding or updating as appropriate). This allows us to reuse the existing outer helpers for this purpose, keeping compilation trackers simpler.