Reduce allocations due to multiple levels of ImmutableArray<FinderLocations> created during find references invocations. #73118
Conversation
…ations> created during find references invocations. This is very similar to this PR (dotnet#73093) that did the same thing for ImmutableArray<Document> in this context.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| foreach (var node in invocations) | ||
| { | ||
| var finderLocation = CreateFinderLocation(node, state, cancellationToken); | ||
| processResult(finderLocation, processResultData); |
There was a problem hiding this comment.
nit: i would be ok with this as a single line.
| @@ -203,7 +203,7 @@ private async Task ProcessProjectAsync(Project project, ImmutableArray<ISymbol> | |||
| { | |||
| await finder.DetermineDocumentsToSearchAsync( | |||
| symbol, globalAliases, project, _documents, | |||
| static (doc, documents) => documents.Add(doc), | |||
| StandardCallbacks<Document>.AddToHashSet, | |||
| @@ -272,12 +272,15 @@ private async Task ProcessProjectAsync(Project project, ImmutableArray<ISymbol> | |||
| // just grab those once here and hold onto them for the lifetime of this call. | |||
| var cache = await FindReferenceCache.GetCacheAsync(document, cancellationToken).ConfigureAwait(false); | |||
|
|
|||
| // scratch hashset to place results in. Populated/inspected/cleared in inner loop. | |||
There was a problem hiding this comment.
this does not look like a hashset :)
| { | ||
| var group = await ReportGroupAsync(symbol, cancellationToken).ConfigureAwait(false); | ||
| await _progress.OnReferenceFoundAsync(group, symbol, location, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| referencesForFinder.Clear(); |
There was a problem hiding this comment.
it's interesting how close this pattern is to the pipewriting/reading stuff i just did. this might get cleaner (in the future) with a channel that is pulling off the items and reporting to them to the progress, while the main find is pushing into the channel.
| // REVIEW: This looks to go to quite a bit of work to populate aliasReferences, but never uses it? | ||
| using var _3 = ArrayBuilder<FinderLocation>.GetInstance(out var aliasReferences); | ||
| await FindLocalAliasReferencesAsync( | ||
| typeReferences, methodSymbol, state, StandardCallbacks<FinderLocation>.AddToArrayBuilder, aliasReferences, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
i think i'm missing where aliasreferences ends up getting used.
There was a problem hiding this comment.
oh. looks like you that was yoru comment as well :D. yes. seems like we can jsut remove this. if no tests break, we're good.
| namedType, state, initialReferences, cancellationToken).ConfigureAwait(false); | ||
| namedType, state, StandardCallbacks<FinderLocation>.AddToArrayBuilder, initialReferences, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| // call processResult on items in initialReferences |
There was a problem hiding this comment.
comment needs fleshing out. maybe like: these shoudl both be reported as normal results, but are also used later on to findmore results.
| } | ||
|
|
||
| initialReferences.AddRange(await FindLocalAliasReferencesAsync( | ||
| initialReferences, symbol, state, cancellationToken).ConfigureAwait(false)); | ||
| // call processResult on items in initialReferences |
| }, | ||
| (state, property, symbol, processResult, processResultData, cancellationToken), | ||
| options with { AssociatePropertyReferencesWithSpecificAccessor = false }, | ||
| cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
this broke me. i assume if tests pass we're good :)
| data.processResult(loc, data.processResultData); | ||
| }, | ||
| processResultData: (options, state, symbol, cancellationToken, self: this, processResult, processResultData), | ||
| cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
this broke me. i assume if tests pass we're good :)
| if (useResult) | ||
| data.processResult(loc, data.processResultData); | ||
| }, | ||
| processResultData: (options, state, symbol, cancellationToken, self: this, processResult, processResultData), |
There was a problem hiding this comment.
can you put 'this' at the front.
There was a problem hiding this comment.
and cancellationToken at the end? keep the rest as much in order as the signature of the containing method.
Mostly stylistic PR feedback
|
Thanks for teh stylistic changes :) |
This is very similar to this PR (#73093) that did the same thing for ImmutableArray in this context. The general idea is to pass an action that the caller can use to process a result, rather than many levels of functions creating and returning ImmutableArrays.