-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Simplify channel-processing code in SemanticSearch. #78060
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.
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
Conversation
|
|
||
| public async IAsyncEnumerable<SyntaxNode> FindAsync(ISymbol symbol) | ||
| { | ||
| var channel = Channel.CreateUnbounded<ReferenceLocation>(new() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to helper in ProducerConsumer that already does the concurrent production/consumption work over an unbounded channel.
| public async IAsyncEnumerable<SyntaxNode> FindAsync(ISymbol symbol) | ||
| { | ||
| var channel = Channel.CreateUnbounded<ReferenceLocation>(new() | ||
| using var _ = PooledHashSet<SyntaxNode>.GetInstance(out var cachedRoots); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also added this so that we don't continually reparse documents with multiple results in them.
|
@tmat this is ready for review. |
|
|
||
| using var _ = cancellationToken.Register( | ||
| static (obj, cancellationToken) => ((Channel<SourceReferenceItem>)obj!).Writer.TryComplete(new OperationCanceledException(cancellationToken)), | ||
| state: channel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProducerConsumer.RunChannelAsync basically has all this code, and is the centralized place where so much of our Producer/Consumer logic goes through. I added a new entrypoint to it to represent this case (where caller just wants to grab the produced results and do transformations on it itself).
| // then convert this to a simple IAsyncEnumerable<ReferenceLocation> that we can iterate over, converting those | ||
| // locations to SyntaxNodes in the corresponding C# or VB document. | ||
| await foreach (var item in ProducerConsumer<ReferenceLocation>.RunAsync( | ||
| FuncReferencesAsync, args: (solution, symbol), cancellationToken)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "FindReferencesAsync"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use lambda?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we generally designed all of these overloads, from the start, to be non-allocating (they get used heavily in some scenarios with lots of callbacks and whatnot). To just simplify the surface area, that means they're all "Delegate + Args". I could def pass a lambda. But there's a certain niceness in just knowing that no delegates (except the first) will actually be allocated :).
No description provided.