Skip to content

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 3, 2024
@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review May 3, 2024 05:52
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 3, 2024 05:52
using var linkedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);

// Defer to the ProducerConsumer. We're search the unreferenced projects in parallel. As we get results, we'll
// add them to the 'allSymbolReferences' queue. If we get enough results, we'll cancel all the other work.
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 whitespace off.

}),
await ProducerConsumer<ImmutableArray<SymbolReference>>.RunParallelAsync(
viableUnreferencedProjects,
produceItems: static async (project, onItemsFound, args) =>
Copy link
Member Author

Choose a reason for hiding this comment

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

this form is much easier to understand (for me at least). instead of hte nested lambdas, you just pass hte single lambda. It will be called in parallel, passing a single item from "viableUnreferencedProjects", and teh callback to call when items are produced based on htat item.

@CyrusNajmabadi CyrusNajmabadi changed the title Provider a ProducerConsumer helper for processing a sequence of items in parallel. Provide a ProducerConsumer helper for processing a sequence of items in parallel. May 3, 2024
@CyrusNajmabadi CyrusNajmabadi requested a review from ToddGrun May 3, 2024 07:49
{
return RunAsync(
// We're running in parallel, so we def have multiple writers
ProducerConsumerOptions.SingleReaderOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

ProducerConsumerOptions.SingleReaderOptions

Why not let the caller specify?

Copy link
Contributor

Choose a reason for hiding this comment

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

never mind, I can see clearly now

Copy link
Member Author

Choose a reason for hiding this comment

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

:-D

ProducerConsumerOptions options,
Func<Action<TItem>, TArgs, Task> produceItems,
Func<ImmutableArray<TItem>, TArgs, Task> consumeItems,
Func<Action<TItem>, TArgs, CancellationToken, Task> produceItems,
Copy link
Contributor

Choose a reason for hiding this comment

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

produceItems

naming nit: maybe produceItem instead?

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 like produceItems. It's the callback responsible for producing all the items. It will just do that by producing them and passing them to callback it itself is provided.

return RunAsync(
// We're running in parallel, so we def have multiple writers
ProducerConsumerOptions.SingleReaderOptions,
produceItems: static (callback, args, cancellationToken) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

I think this might be one level too deep for me to understand without having had more coffee. Local function for the producer here might make things clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Have coffee! I can also extract into local functions on a follow-up if that's ok.

/// <code>Parallel.ForEachAsync</code>, allowing for parallel processing of the items, with a preference towards
/// earlier items.
/// </summary>
private static Task PerformParallelSearchAsync<T>(
Copy link
Contributor

Choose a reason for hiding this comment

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

PerformParallelSearchAsync

Curious if this method provides any value now.

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 can consider removing in follow up

@CyrusNajmabadi
Copy link
Member Author

@ToddGrun can I make those changes in follow-up?

@CyrusNajmabadi CyrusNajmabadi merged commit d01b164 into dotnet:main May 3, 2024
@CyrusNajmabadi CyrusNajmabadi deleted the parallelHelper branch May 3, 2024 16:07
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone May 3, 2024
@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