🐇 Avoid generic enumeration in ChecksumCollection.FindAsync #53959
Conversation
See AB#1333566
| internal static async Task FindAsync<TState>( | ||
| TextDocumentStates<TState> documentStates, | ||
| HashSet<Checksum> searchingChecksumsLeft, | ||
| Dictionary<Checksum, object> result, | ||
| CancellationToken cancellationToken) where TState : TextDocumentState | ||
| { | ||
| foreach (var state in documentStates.States) | ||
| foreach (var (_, state) in documentStates.States) |
There was a problem hiding this comment.
📝 The generic enumeration was responsible for 89% of the total allocations in searching for assets (10% of the total for the entire VS recording). This fixes a 16.10 regression introduced in #51526.
| => projectState.DocumentStates.States | ||
| .Concat(projectState.AdditionalDocumentStates.States) | ||
| .Concat(projectState.AnalyzerConfigDocumentStates.States); | ||
| => projectState.DocumentStates.States.Values |
There was a problem hiding this comment.
There was a problem hiding this comment.
➡️ This line is the same in this pull request as it was before this pull request. The performance improvement was applied targeting the single hot path method which is now marked with [PerformanceSensitive]. Other methods were optimized where trivial, or left the same.
| @@ -83,8 +83,8 @@ public TState GetRequiredState(DocumentId documentId) | |||
| /// <summary> | |||
| /// States ordered by <see cref="DocumentId"/>. | |||
| /// </summary> | |||
| public IEnumerable<TState> States | |||
| => _map.Values; | |||
There was a problem hiding this comment.
Add ImmutableSortedDictionary.Values to banned APIs?
There was a problem hiding this comment.
We could also make this a struct enumerator and keep the callers as they are.
There was a problem hiding this comment.
Add ImmutableSortedDictionary.Values to banned APIs?
I wouldn't go this far. The PerformanceSensitive attribute is designed with the intent of statically validating this case where it matters.
We could also make this a struct enumerator and keep the callers as they are.
This is a bit difficult. I added these struct enumerators when I wrote ImmutableSortedTreeDictionary<TKey, TValue> but it would be a binary breaking change to incorporate it back into System.Collections.Immutable. See dotnet/runtime#13927 for more details.
There was a problem hiding this comment.
Yeah, I understand we can't update ImmutableSortedTreeDictionary, but we can either create an extension method GetValues(ImmutableSortedTreeDictionary<K,V>) or make specialized struct enumerator just for this property.
sharwell
changed the title
ghost
added this to the 
See AB#1333566