Cleanup how we create skeleton assemblies. #57655
Conversation
| private static readonly ConditionalWeakTable<Compilation, SkeletonReferenceSet> s_compilationToReferenceMap = new(); | ||
|
|
||
| private readonly object _gate = new(); | ||
| private partial class CachedSkeletonReferences |
There was a problem hiding this comment.
this moved inside SolutionState so it could refer to ICompilationTracker.
CyrusNajmabadi
force-pushed
the
skeletonCleanup
branch
from
273c3ce
to
8dae3c9
Compare
|
@dibarbet can you ptal? |
| var metadataReference = TryGetReferenceSet(version)?.GetMetadataReference(properties); | ||
| if (metadataReference != null) | ||
| { | ||
| workspace.LogTestMessage($"Reusing the already cached skeleton assembly for {compilationTracker.ProjectState.Id}"); |
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); |
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); |
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); |
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 |
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. |
Yup. I hate the CWT (and the logic around it). So it's going in for sure. |
ghost
added this to the
fairly certain there is no race in the new PR> we'll look at that together. |
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.