Cache indices against our PEReference object, not the underlying metadata ID #64742
Conversation
| /// to want to search the same metadata simultaneously. As such, we use an AsyncLazy to compute the value that | ||
| /// can be shared among all callers. | ||
| /// </summary> | ||
| private static readonly ConditionalWeakTable<MetadataId, AsyncLazy<SymbolTreeInfo>> s_metadataIdToInfo = new(); |
| @@ -26,7 +26,7 @@ namespace Microsoft.CodeAnalysis.FindSymbols.SymbolTree | |||
| internal sealed partial class SymbolTreeInfoCacheService : IWorkspaceService | |||
| { | |||
| private readonly ConcurrentDictionary<ProjectId, SymbolTreeInfo> _projectIdToInfo = new(); | |||
| private readonly ConcurrentDictionary<MetadataId, MetadataInfo> _metadataIdToInfo = new(); | |||
There was a problem hiding this comment.
this was not safe, different PortableExecutableReferences can have hte same MetadataId.
There was a problem hiding this comment.
this could lead to removing the mapping once one of several PERefs was removed that had the same ID.
| reference, | ||
| id => new AsyncLazy<SymbolTreeInfo>( | ||
| c => CreateMetadataSymbolTreeInfoAsync(services, solutionKey, reference, checksum, c), | ||
| cacheResult: true)); |
There was a problem hiding this comment.
this cached from PERef -> SymbolTreeInfo. Every different PERef will have a unique SymoblTreeInfo with it's own unique checksum. though each of those infos may reuse the rest of the data from another SymbolTreeInfo.
Another way to think about this would that there's just the raw underlying data, and then a wrapper around that that points at the raw data and also the checksum. the raw data can/should be shared across all PERefs with the same meata. The data+checksum is always associated with a unique PERef though
There was a problem hiding this comment.
i still may have a followup that actually splits the type into just the raw data, and then the raw-data+checksum. but i considered that too large to do here.
There was a problem hiding this comment.
I like the idea, that WithChecksum bit does feel a little hacky :)
| Assert.Equal(info2.Checksum, checksum2); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
validated failures before change, and success after.
| /// to want to search the same metadata simultaneously. As such, we use an AsyncLazy to compute the value that | ||
| /// can be shared among all callers. | ||
| /// </summary> | ||
| private static readonly ConditionalWeakTable<MetadataId, AsyncLazy<SymbolTreeInfo>> s_metadataIdToInfo = new(); |
There was a problem hiding this comment.
moved to different file.
| /// computation of the <see cref="SymbolTreeInfo"/> once per <see cref="MetadataId"/>, but we may then have to | ||
| /// make a copy of it with a new <see cref="Checksum"/> if the checksums differ. | ||
| /// </summary> | ||
| private static readonly ConditionalWeakTable<MetadataId, AsyncLazy<SymbolTreeInfo>> s_metadataIdToSymbolTreeInfo = new(); |
There was a problem hiding this comment.
this is the crux of hte change and the thing i missed which was causing the problem. THere can ne a many->one mapping from PERefs to a particular MetadataId. But i was keying only off metadata-id, assuming it would be 1:1.
| if (info.Checksum == checksum) | ||
| return info; | ||
| Contract.ThrowIfTrue(info.Checksum != checksum, "How could the info stored for a particular PEReference now have a different checksum?"); | ||
| return info; |
There was a problem hiding this comment.
this contract needs to be true. if we're keying by PERef we should always get the same checksum (as that is determined purely by the PERef)
ghost
added this to the 
I broke this in #63449
The core issue here is that we base our Checksum off of the PEReference, but cache using a key based off a MetadataId. Two PERefs can have the MetadataId, but different checksums. For example, if you just change the metadata-properties of a PERef (like the 'Aliases' it has, or if interop types are embedded), then teh metadata for it stays the same.
This was problematic as we were assuming that if the metadataId was the same, you'd always have the same checksum.
Working on test now.