Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve processing in FindRefs #73253

Merged
merged 47 commits into from
Apr 28, 2024
Merged

Improve processing in FindRefs #73253

merged 47 commits into from
Apr 28, 2024

Conversation

CyrusNajmabadi
Copy link
Member

Similar to what was done in #73249.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 27, 2024
CyrusNajmabadi
CyrusNajmabadi commented Apr 27, 2024
View reviewed changes
src/Features/Core/Portable/CodeLens/CodeLensFindReferenceProgress.cs Outdated
@@ -58,14 +58,6 @@ public void OnCompleted()
{
}

public void OnFindInDocumentStarted(Document document)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FAR had a system to notify consumers "i'm startign a doc" and "i'm done with a doc". The original thinking was to use that for progress, to display which file we're looking at.

but:

  1. we never hooked this up to naything at all.
  2. this wouldn't be a good experience anyways, since we're extremely parallel. so this was removed entirely.

CyrusNajmabadi
CyrusNajmabadi commented Apr 27, 2024
View reviewed changes
src/Features/Core/Portable/FindUsages/AbstractFindUsagesService.ProgressAdapter.cs
@@ -107,17 +106,21 @@ public async ValueTask OnDefinitionFoundAsync(SymbolGroup group, CancellationTok
await context.OnDefinitionFoundAsync(definitionItem, cancellationToken).ConfigureAwait(false);
}

public async ValueTask OnReferenceFoundAsync(SymbolGroup group, ISymbol definition, ReferenceLocation location, CancellationToken cancellationToken)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

view with whitespcae off.

CyrusNajmabadi
CyrusNajmabadi commented Apr 27, 2024
View reviewed changes
src/Features/Core/Portable/GoToBase/AbstractGoToBaseService.cs
@@ -44,7 +44,7 @@ public async Task FindBasesAsync(IFindUsagesContext context, Document document,
var (symbol, project) = symbolAndProjectOpt.Value;

var solution = project.Solution;
var bases = await FindBaseHelpers.FindBasesAsync(symbol, solution, cancellationToken).ConfigureAwait(false);
var bases = FindBaseHelpers.FindBases(symbol, solution, cancellationToken);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a lot of FAR and helpers becamse sync. will explain how later in pr.

CyrusNajmabadi
CyrusNajmabadi commented Apr 27, 2024
View reviewed changes
src/Features/Core/Portable/FindUsages/AbstractFindUsagesService.ProgressAdapter.cs
@@ -73,8 +74,6 @@ public IStreamingProgressTracker ProgressTracker
// any of these.
public ValueTask OnStartedAsync(CancellationToken cancellationToken) => default;
public ValueTask OnCompletedAsync(CancellationToken cancellationToken) => default;
public ValueTask OnFindInDocumentStartedAsync(Document document, CancellationToken cancellationToken) => default;
public ValueTask OnFindInDocumentCompletedAsync(Document document, CancellationToken cancellationToken) => default;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FAR had a system to notify consumers "i'm startign a doc" and "i'm done with a doc". The original thinking was to use that for progress, to display which file we're looking at.

but:

we never hooked this up to naything at all.
this wouldn't be a good experience anyways, since we're extremely parallel. so this was removed entirely.

ToddGrun
ToddGrun reviewed Apr 28, 2024
View reviewed changes
src/Workspaces/Core/Portable/FindSymbols/SymbolFinder_Hierarchy.cs
@@ -54,22 +54,20 @@ public static partial class SymbolFinder
var sourceMember = await FindSourceDefinitionAsync(m, solution, cancellationToken).ConfigureAwait(false);
var bestMember = sourceMember ?? m;

if (await IsOverrideAsync(solution, bestMember, symbol, cancellationToken).ConfigureAwait(false))
{
if (IsOverride(solution, bestMember, symbol))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (IsOverride(solution, bestMember, symbol))

nit: ArrayBuilder could use disposer like other place do

ToddGrun
ToddGrun reviewed Apr 28, 2024
View reviewed changes
src/Workspaces/Core/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine.cs
@@ -114,20 +180,13 @@ public Task FindReferencesAsync(ISymbol symbol, CancellationToken cancellationTo
// which is why we do it in this loop and not inside the concurrent project processing that happens
// below.
await symbolSet.InheritanceCascadeAsync(currentProject, cancellationToken).ConfigureAwait(false);
allSymbols = symbolSet.GetAllSymbols();
var allSymbols = symbolSet.GetAllSymbols();

// Report any new symbols we've cascaded to to our caller.
await ReportGroupsAsync(allSymbols, cancellationToken).ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this can't be moved into the parallel part of the computation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me noodle on that. it would be clunkier. Fundamentally, the inheritance cascade is serial. The reporting of results it finds is... not that important. but the reporting does need to happen prior to any symbols for a group being fired.

ToddGrun
ToddGrun reviewed Apr 28, 2024
View reviewed changes
src/Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/AbstractReferenceFinder.cs Outdated
@@ -380,7 +376,7 @@ protected static Task FindDocumentsWithForEachStatementsAsync<TData>(Project pro
CancellationToken cancellationToken)
{
var document = state.Document;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document

not used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

ToddGrun
ToddGrun approved these changes Apr 28, 2024
View reviewed changes
Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Apr 28, 2024
Merged
@CyrusNajmabadi CyrusNajmabadi merged commit 7a96b15 into dotnet:main Apr 28, 2024
25 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 28, 2024
@CyrusNajmabadi CyrusNajmabadi deleted the farParallel branch April 28, 2024 14:31
@dibarbet dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ToddGrun ToddGrun ToddGrun approved these changes

Assignees
No one assigned
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.11 P1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@CyrusNajmabadi @ToddGrun @dibarbet