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.

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

Add helper for common pattern of producing an array. #73336

Merged
merged 33 commits into from
May 5, 2024

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 3, 2024
@@ -101,7 +102,7 @@ private static void ClearCachedData()
ImmutableArray<DocumentKey> priorityDocumentKeys,
string searchPattern,
IImmutableSet<string> kinds,
Func<ImmutableArray<RoslynNavigateToItem>, VoidResult, CancellationToken, Task> onItemsFound,
Func<IAsyncEnumerable<RoslynNavigateToItem>, VoidResult, CancellationToken, Task> onItemsFound,
Copy link
Member Author

Choose a reason for hiding this comment

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

i was getting into overload explosion. So i changes a lot of hte core helpers to be all IAsyncEnumerable based

await foreach (var (docId, nodeOrText) in results)
docIdToNewRootOrText[docId] = nodeOrText;

return docIdToNewRootOrText;
Copy link
Member Author

Choose a reason for hiding this comment

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

instead of neeing this to be on the outsdie, captured in 'args', we now just allow consumeItems to create a final value which is the value of the entire producer/consumer operation.

@@ -82,27 +82,27 @@ public abstract class DiagnosticProvider
return ImmutableDictionary.CreateRange([KeyValuePairUtil.Create(project, diagnostics)]);

case FixAllScope.Solution:
var projectsAndDiagnostics = ImmutableDictionary.CreateBuilder<Project, ImmutableArray<Diagnostic>>();
Copy link
Member Author

Choose a reason for hiding this comment

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

same as above, this moves into consumeItems now.

@@ -94,12 +94,10 @@ public sealed override IEnumerable<FixAllScope> GetSupportedFixAllScopes()

using var _1 = progressTracker.ItemCompletedScope();

var docIdToNewRootOrText = new Dictionary<DocumentId, (SyntaxNode? node, SourceText? text)>();
Copy link
Member Author

Choose a reason for hiding this comment

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

moved inside consumeItems.

args: (source, produceItems, consumeItems, args),
cancellationToken);
// Bridge to sibling helper that operates on an IAsyncEnumerable.
return RunParallelAsync(source.AsAsyncEnumerable(), produceItems, consumeItems, args, 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.

was able to get several overloads to be much simpler by having the iAsyncEnumerable forms be the primary ones, and the IEnumerable forms just trivially defer to that.

consumeItems: static async (items, args, cancellationToken) =>
{
await args.consumeItems(items, args.args, cancellationToken).ConfigureAwait(false);
return default(VoidResult);
Copy link
Member Author

Choose a reason for hiding this comment

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

similarly, the Task based mthods work by deferring to the Task version, using VoidResult as the ignored type arg.

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review May 4, 2024 00:18
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 4, 2024 00:18
// Bridge to sibling helper that takes a consumeItems that returns a value.
return RunAsync(
options,
produceItems: static (callback, args, cancellationToken) => args.produceItems(callback, args.args, cancellationToken),
Copy link
Contributor

@ToddGrun ToddGrun May 4, 2024

Choose a reason for hiding this comment

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

static (callback, args, cancellationToken) => args.produceItems(callback, args.args, cancellationToken),

Early, so coffee might not have hit, but could this just be the following instead?

produceItems: produceItems,

(I guess this question applies for other places in the file that do the same)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. Notice the args.args part. The two delegates have definite different shapes.

ReadFromChannelAndConsumeItemsAsync()).ConfigureAwait(false);
var writeTask = ProduceItemsAndWriteToChannelAsync();
var readTask = ReadFromChannelAndConsumeItemsAsync();
await Task.WhenAll(writeTask, readTask).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.

await Task.WhenAll(writeTask, readTask).ConfigureAwait(false);

nit: as we're at the bowels, I feel like not allocating the array here is simple and worthwhile. Instead of doing the WhenAll, just awaiting writeTask will work the same without allocating the array, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

tecahnically we don't even need to await 'writeTask'. we could just return 'readTask'. but i like the clarity of this WhenAll :)

return await RunParallelAsync(
source,
produceItems: static (item, callback, args, cancellationToken) => args.produceItems(item, callback, args.args, cancellationToken),
consumeItems: static (stream, args, cancellationToken) => stream.ToImmutableArrayAsync(cancellationToken),
Copy link
Contributor

Choose a reason for hiding this comment

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

stream

This threw me for a loop, maybe name something different

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 enabled auto-merge May 4, 2024 17:53
@CyrusNajmabadi CyrusNajmabadi merged commit d2f7849 into dotnet:main May 5, 2024
25 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone May 5, 2024
@CyrusNajmabadi CyrusNajmabadi deleted the produceArray branch May 5, 2024 04:28
@CyrusNajmabadi
Copy link
Member Author

@jasonmalinowski For review when you get back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants