Cleanup how we create skeleton assemblies.#57655
Merged
CyrusNajmabadi merged 8 commits intodotnet:mainfrom Nov 10, 2021
Merged
Conversation
CyrusNajmabadi
commented
Nov 9, 2021
| private static readonly ConditionalWeakTable<Compilation, SkeletonReferenceSet> s_compilationToReferenceMap = new(); | ||
|
|
||
| private readonly object _gate = new(); | ||
| private partial class CachedSkeletonReferences |
Contributor
Author
There was a problem hiding this comment.
this moved inside SolutionState so it could refer to ICompilationTracker.
273c3ce to
8dae3c9
Compare
Contributor
Author
|
@dibarbet can you ptal? |
dibarbet
reviewed
Nov 10, 2021
| var metadataReference = TryGetReferenceSet(version)?.GetMetadataReference(properties); | ||
| if (metadataReference != null) | ||
| { | ||
| workspace.LogTestMessage($"Reusing the already cached skeleton assembly for {compilationTracker.ProjectState.Id}"); |
Member
There was a problem hiding this comment.
interesting, neve rseen LogTestMessage before - where do those messages go?
| // being passed in. If so, we can just reuse what we already computed before. | ||
| var workspace = solution.Workspace; | ||
| var version = await compilationTracker.GetDependentSemanticVersionAsync(solution, cancellationToken).ConfigureAwait(false); | ||
| var metadataReference = TryGetReferenceSet(version)?.GetMetadataReference(properties); |
Member
There was a problem hiding this comment.
the caching that is going on here is a bit confusing.
- we first look at the
_skeletonReferenceSetfromTryGetReferenceSet - Assuming that isn't present, it goes to
GetOrBuildReferenceSetAsyncwhich immediately looks at thes_compilationToReferenceMap(inTryGetExistingReferenceSet) for the compilation with a note mentioning that we want to return the same instance for a compilation. Should 2) be done before 1)? Or is 2) done after to avoid looking at the compilation if not needed? TryGetExistingReferenceSetthen callsTryGetReferenceSet, but we already looked at that in 1). Something doesn't add up
To me it is quite unclear which cache is preferred and when an item should be in s_compilationToReferenceMap vs. _skeletonReferenceSet
| if (referenceSet != null) | ||
| return referenceSet; | ||
|
|
||
| storage = TryCreateMetadataStorage(workspace, compilation, cancellationToken); |
Member
There was a problem hiding this comment.
I think there may be a race here?
- threadA hits line 163, enters the semaphore and starts creating the storage
- threadB hits line 163 while 1) is running and starts waiting
- threadA finishes creating the storage, exits the semaphore, but doesn't yet return the new skeleton references / update the fields
- threadB enters the semaphore, doesn't see the result from 3) since it isn't updated, and starts creating the same storage again.
Maybe the updating of the locals needs to happen in the semaphore as well
| { | ||
| private readonly object _gate = new(); | ||
| // Then see if we already have a reference set for this version. if so, we're done and can return that. | ||
| return TryGetReferenceSet(version); |
Member
There was a problem hiding this comment.
as above the ordering seems weird. sometimes TryGetReferenceSet is called before the s_compilationToReferenceMap
|
|
||
| using (Logger.LogBlock(FunctionId.Workspace_SkeletonAssembly_EmitMetadataOnlyImage, cancellationToken)) | ||
| { | ||
| // TODO: make it to use SerializableBytes.WritableStream rather than MemoryStream so that |
Member
There was a problem hiding this comment.
does this todo apply? looks like it's using SerializableBytes
| /// <summary> | ||
| /// Use WeakReference so we don't keep MetadataReference's alive if they are not being consumed. | ||
| /// Note: if the weak-reference is actuall <see langword="null"/> (not that it points to null), | ||
| /// that means |
| /// that means | ||
| /// Note: if the weak-reference is actually <see langword="null"/> (not that it points to null), | ||
| /// that means we know we were unable to generate a reference for those properties, and future | ||
| /// calls can early exit. |
dibarbet
approved these changes
Nov 10, 2021
Contributor
Author
Yup. I hate the CWT (and the logic around it). So it's going in for sure. |
Contributor
Author
fairly certain there is no race in the new PR> we'll look at that together. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Will comment the overall thrust of hte changes in the PR.
THis should be viewed with whitespace off.
Should be reviewed a commit at a time.