Update FAR engine to be able to work without having access to all projects at the same time.#54654
Conversation
dibarbet
left a comment
There was a problem hiding this comment.
this is one PR where I wish richnav PR indexing worked!
src/Workspaces/Core/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine.SymbolSet.cs
Show resolved
Hide resolved
src/Workspaces/Core/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine.SymbolSet.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine.SymbolSet.cs
Outdated
Show resolved
Hide resolved
| /// </summary> | ||
| /// <remarks> | ||
| /// This method is non threadsafe as it mutates the symbol set instance. As such, it should only be | ||
| /// called serially. <see cref="GetAllSymbols"/> should not be called concurrently with this. |
There was a problem hiding this comment.
would it be worth implementing these methods with a lock / some kind of assert in here verifying that no one is accidentally calling these methods concurrently, then delegate to other abstract methods?
There was a problem hiding this comment.
Yes. I can add a poor man's assert that only one is being called at a time.
There was a problem hiding this comment.
I like this idea. Maybe only in Debug though?
src/Workspaces/Core/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine.cs
Show resolved
Hide resolved
src/Workspaces/Core/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine.cs
Outdated
Show resolved
Hide resolved
ryzngard
left a comment
There was a problem hiding this comment.
I still need to look at src/Workspaces/Core/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine.cs changes, but brain can only do so much this late
| // First find all the projects that could potentially reference this type. | ||
| var projectsThatCouldReferenceType = await GetProjectsThatCouldReferenceTypeAsync( | ||
| type, solution, searchInMetadata, cancellationToken).ConfigureAwait(false); | ||
| List<Project> orderedProjectsToExamine; |
There was a problem hiding this comment.
nit: I know it's generally bad to use IEnumerable<T> instead of a concrete class, but this might be a case where it makes more sense. It would help avoid the ToList call and we only need it to iterate. Is there a reason to use List<T> here instead?
There was a problem hiding this comment.
this is a good question. The reason is more philosophical than anything else. I'm generally wary of IEnumerables as it can be very hard to gauge certain things (for example: how expensive htey are, or if they return teh same results when querying them multiple times). A list is a well known construct that i can be sure is computed and will give me consistent results.
| /// </summary> | ||
| /// <remarks> | ||
| /// This method is non threadsafe as it mutates the symbol set instance. As such, it should only be | ||
| /// called serially. <see cref="GetAllSymbols"/> should not be called concurrently with this. |
There was a problem hiding this comment.
I like this idea. Maybe only in Debug though?
| return result; | ||
| } | ||
|
|
||
| private static async Task<HashSet<ISymbol>> DetermineInitialUpSymbolsAsync( |
There was a problem hiding this comment.
Are there cases where we add up symbols that aren't part of the initial? Like through linked symbols?
There was a problem hiding this comment.
yes. a simply example is if you start on an override. The 'initial up' symbols will contain the member that this override overrode (And everything higher).
src/Workspaces/Core/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine.SymbolSet.cs
Show resolved
Hide resolved
src/Workspaces/Core/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine.SymbolSet.cs
Outdated
Show resolved
Hide resolved
...re/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine.UnidirectionalSymbolSet.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/FindSymbols/SymbolFinder_Helpers.cs
Outdated
Show resolved
Hide resolved
dibarbet
left a comment
There was a problem hiding this comment.
I think mainly looks good to me!
…arOrder2" (dotnet#54874)" This reverts commit a129046.
Note: tehre is a large amount of churn in this PR. I recommend anyone reviewing it potentially discuss things with me first to get a sense of what is going on. I will also attempt to doc what's going on the best that i can.
--
This PR heavily changes the way that the FAR engine works. To help understand what has changed, it's worth going through how the existing engine behaves. Specifically, how it behaves with respect to finding symbols in a particular symbols' inheritance hierarchy.
To see where this occurs, imagine you have:
When searching for either IFoo.Foo or C.Foo, FAR must 'cascade' to the other as the user may have calls to either in their solution, but all those results should be returned as without htem the user may not know about relevant calls that might end up calling into the member in question they care about.
To support this, teh FAR engine today first walks the entire symbolic inheritance hierarchy for a symbol looking for all these connections. This necessarily mandates looking at all projects to find all of these. Then, once all the symbols are found, we use those to determine our plan for searching all the relevant projects and documents in teh solution for actual matches.
This is two-fold problematic. First, you can't get any results from FAR until we've done that first full solution scan. Second, it requires us to actually be able to see teh whole solution to search it for the inheritance symbols. This is particularly problematic as we work toward the snappy-design (where we will not have projects loaded for code the user isn't directly working with).
--
With that out of the way, here's how the new engine works. Instead of first scanning the entire solution to figure out the inheritance hierarchy, we instead do it piece-meal. Specifically, given a starting symbol we can always trivially find the symbols above it in the inheritance hierarchy. These are the symbols that it inherits from, and it must be in the same project or a project it references (both of which will be loaded for the user). Given those symbols, we can determine the set of dependent projects that could end up referencing these.
We then process those dependent projects individually. As we get to each project we can then scan it to determine what new symbols may be in that project that derive from teh symbols being searched for. Once we've determined that, we can then add that project to the queue of projects that the engine can scan concurrently with the rest.
--
Currently, iterating through the dependent projects is still handled by Roslyn. However, this sets us up for a future where we allow dependent project iteration to be handled by an external service. For example, you could imagine services provided in VS to walk the set of projects loading them, searching them, and then unloading them to keep resource utilization low.