Move skeleton ref caching to ProjectStates#57158
Move skeleton ref caching to ProjectStates#57158CyrusNajmabadi merged 26 commits intodotnet:mainfrom
Conversation
32f6500 to
8ff6219
Compare
| using Microsoft.CodeAnalysis.Host; | ||
| using Roslyn.Utilities; | ||
|
|
||
| namespace Microsoft.CodeAnalysis |
There was a problem hiding this comment.
note: this was previously called MetadataOnlyReferences. The spirit of the type remains mostly the same, though it is heavily rewritten. Core to the change is that instead of just being static free-floating CWT data keyed off of BranchId+ProjectId, it is now instance data associated with ProjectState instances.
…References.cs Co-authored-by: Jason Malinowski <jason@jason-m.com>
| /// downstream consumers are only likely to resolve a small handful of these symbols in practice, this | ||
| /// should not be expensive to hold onto. Importantly, semantic models and complex method binding/caching | ||
| /// should never really happen with this compilation. | ||
| /// </summary> |
There was a problem hiding this comment.
this design greatly simplifies later code. it means we can proactively create a DocProvider and cache a single isntance for all usages. this avoids hte need for complex paths in the compilation tracker that have to attempt to use decl-compilations if possible, and it avoids complexity in the skeleton ref cache relating to trying to handle decl compilatiosn instead of just final compilations.
| // another thread may have come in and beaten us to computing this. So attempt to actually cache this | ||
| // in the global map. if it succeeds, use our computed version. If it fails, use the one the other | ||
| // thread succeeded in storing. | ||
| referenceSet = s_compilationToReferenceMap.GetValue(finalCompilation, _ => referenceSet); |
There was a problem hiding this comment.
i separated out all handling of s_compilationToReferenceMap. this is hte only method that touches it. all other methods only reference the state of this object along.
| // guarantees only one thread is building at a time | ||
| private readonly SemaphoreSlim _buildLock = new(initialCount: 1); | ||
|
|
||
| public CachedSkeletonReferences CachedSkeletonReferences { get; } |
There was a problem hiding this comment.
other option was to store this on the state object. however, that didn't seem necessary/desirable, so i kept things like this.
There was a problem hiding this comment.
If it's necessary it's not obvious to me why it would be, so seems fine.
…GeneratedFileReplacingCompilationTracker.cs
|
@jasonmalinowski this is ready for another pass. |
jasonmalinowski
left a comment
There was a problem hiding this comment.
The implementation seems deceptively simple now, I like it. I did have one potential concern about reuse, but if you think it's not a scenario that can happen then happy to sign off.
I admit I don't know if that conditional weak table is doing anything for us anymore; it seems as long as the compilation is still reachable in some way (which means the compilation tracker for that project instance is still around), we'd be rooting the SkeletonReferenceSet anyways. I'm happy for you to try deleting it in a follow up PR if you want to go ahead with this since this is already a vast improvement over what was there.
And most importantly: do we feel the existing tests are covering everything well here? I know a lot of our tests implicitly use skeleton refs which is why a bunch of this code has that logging when we did break it once in a subtle way. Not sure we have much that actually tests it directly though.
| /// in the ID to be created. As downstream consumers are only likely to resolve a small handful of these | ||
| /// symbols in practice, this should not be expensive to hold onto. Importantly, semantic models and | ||
| /// complex method binding/caching should never really happen with this compilation. | ||
| /// </summary> |
| using System.Threading; | ||
| using Microsoft.CodeAnalysis.Host; | ||
|
|
||
| namespace Microsoft.CodeAnalysis; |
There was a problem hiding this comment.
Oh this is going to take some getting used to.... 😄 (I first saw this file and went "what's with the indenting???")
| // guarantees only one thread is building at a time | ||
| private readonly SemaphoreSlim _buildLock = new(initialCount: 1); | ||
|
|
||
| public CachedSkeletonReferences CachedSkeletonReferences { get; } |
There was a problem hiding this comment.
If it's necessary it's not obvious to me why it would be, so seems fine.
jasonmalinowski
left a comment
There was a problem hiding this comment.
Chatting with @CyrusNajmabadi he felt the potential concern about non-top-level edits should be fine, since the expectation is the compiler is always avoiding method bodies when emitting skeletons.
This is part of the work to get us off of using BranchIds.
The new code is clearer and better assocaited with its corresponding workspace state.