Perform progression search in two phases. #58785
Conversation
| // that result might not contain full data | ||
| // Ensure that we actually complete all our remaining progress items so that the progress bar completes. | ||
| await ProgressItemsCompletedAsync(_remainingProgressItems, cancellationToken).ConfigureAwait(false); | ||
| Debug.Assert(_remainingProgressItems == 0); |
There was a problem hiding this comment.
it was becoming a PITA for all the control flow to keep track of this and set progress to the 'finished' state at teh end. So now we just keep track ourselves across all codepaths and just jump to teh end unilaterally.
| [Flags] | ||
| internal enum NavigateToSearchScope | ||
| { | ||
| RegularDocuments = 0b01, |
There was a problem hiding this comment.
where do the enum values come from?
There was a problem hiding this comment.
Not sure what you mean. Can you clarify?
There was a problem hiding this comment.
Ah - I meant why the value 0b01?
There was a problem hiding this comment.
Ah. It's a flags enum. So I prefer using binary to show the values and how they can be combined.
| // providing this extra information will make UI to show indication to users | ||
| // that result might not contain full data | ||
| // Ensure that we actually complete all our remaining progress items so that the progress bar completes. | ||
| await ProgressItemsCompletedAsync(_remainingProgressItems, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
so is the only case where _remainingProgressItems would not be zero is when an exception is thrown and we aren't able to complete the search?
There was a problem hiding this comment.
not sure how possible this is, but could this race? For example we have 5 searches remaining, one document throws triggering this catch (we subtract 5 from the remaining), but then one of the previously in progress document searches also completes, subtracting another 1 and we get -1.
There was a problem hiding this comment.
so is the only case where _remainingProgressItems would not be zero is when an exception is thrown and we aren't able to complete the search?
No. A totally normal thing that causes this is if we also shortcircuit remaining work. i can add comment, btu this happens if we search cached documents and find results (during solution load). in that case, we skip the other searches.
There was a problem hiding this comment.
not sure how possible this is, but could this race?
It should not. This is async-serial. Meaning this should never run concurrently with any finding work.
| } | ||
|
|
||
| await SearchFullyLoadedProjectsAsync(orderedProjects, seenItems, cancellationToken).ConfigureAwait(false); | ||
| await SearchGeneratedDocumentsAsync(seenItems, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
since this is streaming, wouldn't we have reported the non source generated results first already?
There was a problem hiding this comment.
i'm not sure what you mean :)
| // Create two queries. One to find results in normal docs, and one to find results in generated | ||
| // docs. That way if the generated docs take a long time we can still report the regular doc | ||
| // results immediately. | ||
| graphQueries.Add(new SearchGraphQuery(searchParameters.SearchQuery.SearchString, NavigateToSearchScope.RegularDocuments, threadingContext, asyncListener)); |
There was a problem hiding this comment.
similar question as above, the code was already searching non-source generated first and since we're streaming results back wouldn't they have been reported first? why do we need to explicitly separate the search?
There was a problem hiding this comment.
similar question as above, the code was already searching non-source generated first and since we're streaming results back wouldn't they have been reported first? why do we need to explicitly separate the search?
Ah, i see the question. Progression is not streaming. We have to grab the produced-sub-graph (what we make) and apply it at once to the full graph. So this allows us to break that into two chunks. The produced-sub-graph for teh normal docs, and the produced-sub-graph for the generated docs. we can apply each to the larger graph when each finishes.
Co-authored-by: David Barbet <dibarbet@gmail.com>
ghost
added this to the 
Partial fix for:
AB#1431669 and
First phase searches normal documents. Second phase searches generated docs. This allows progression search to still show the full set of matches, while not forcing the full results to come once the generated results are computed.
This is a partial fix as we still need to update the tests here. Specifically, the total time to do the full search has gotten longer (As we are generating and searching generated docs). However, we should be measuring time to get the initial set of docs as that will still be as fast as nav-to is with the normal indices and caches.