Skip to content

Conversation

@CyrusNajmabadi
Copy link
Contributor

With this change, if you ask a Document instance multiple times for its frozen snapshot, you'll always get back the same instance. Previously, you'd always get a fresh solution instance back.

This means that you wouldn't ever benefit from things like being able to share data cached on a Document instance (like the SemanticModel for example). So if you had N features waking up, and freezing a Doc, and then getting sematnic models, they'd all operate on different instance. Now, they'll operate on the same instance, as long as one of htem is currently keeping it alive.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner February 20, 2024 23:16
@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 20, 2024
/// Mapping of DocumentId to the frozen solution we produced for it the last time we were queried. This
/// instance should be used as its own lock when reading or writing to it.
/// </summary>
private readonly Dictionary<DocumentId, AsyncLazy<Solution>> _documentIdToFrozenSolution = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note:

  1. i've been burned by ConcurrentDictionary in the past. So i prefer just a normal dictionary with locking. Could revisit this if we see any issues.
  2. currently, it's just using normal locking. we could consider cancellable locking with a semaphore slim if we see any issues.

lazySolution = CreateLazyFrozenSolution(this.CompilationState, documentId);
_documentIdToFrozenSolution.Add(documentId, lazySolution);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be cheap/fast, as we're not doing anything beyond looking up or creating the lazy (not the actual computation).

{
return new AsyncLazy<Solution>(
cancellationToken => Task.FromResult(ComputeFrozenSolution(compilationState, documentId, cancellationToken)),
cancellationToken => ComputeFrozenSolution(compilationState, documentId, cancellationToken));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a bit of an odd pattern that's been arising recently. All the actual computation work is async, but we're storing it in an AsyncLazy. This is so that we have the nice benefit that N callers can be trying to get the value at the same time, but only one will compute it. Also, if all of htem cancel, the underlying work cancels. (as opposed to if N callers are asking, and the one computing ends up cancelling, causing the next that is waiting to have to redo all the work).

/// <summary>
/// Result of calling <see cref="WithFrozenPartialCompilationsAsync"/>.
/// </summary>
private AsyncLazy<Solution> CachedFrozenSolution { get; init; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now all teh cached state is always at the Solution level, not the SolutionCompilationState level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since every new SolutionCompilationState would always produce a new Solution, this was not useful to have in the inner layer (especially since everyone externally only works with the outher Solution/Document types).

/// </summary>
private readonly Dictionary<DocumentId, SolutionCompilationState> _cachedFrozenDocumentState = [];

private readonly AsyncLazy<SolutionCompilationState> _cachedFrozenSnapshot;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed both the document map, and the cached frozen frozen solution-level state. both concept are now entirely held in teh Solution level.

@CyrusNajmabadi CyrusNajmabadi merged commit 6eb91fb into dotnet:main Feb 21, 2024
@ghost ghost added this to the Next milestone Feb 21, 2024
@CyrusNajmabadi CyrusNajmabadi deleted the frozenSolution branch February 21, 2024 01:34
@jjonescz jjonescz modified the milestones: Next, 17.10 P2 Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants