-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Port runtime's Parallel.ForEachAsync so we can use it on NetFx. #73332
Conversation
src/Workspaces/Core/Portable/Shared/Utilities/RoslynParallel.NetFramework.cs
Outdated
Show resolved
Hide resolved
#if false | ||
ArgumentNullException.ThrowIfNull(source); | ||
ArgumentNullException.ThrowIfNull(body); | ||
#endif |
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.
for now, just disabling the arg checks internally. we rarely ever check this with our ienumerable helpers.
// Convenience property used by TPL logic | ||
private static TaskScheduler EffectiveTaskScheduler(ParallelOptions options) => options.TaskScheduler ?? TaskScheduler.Current; | ||
|
||
private static int EffectiveMaxConcurrencyLevel(ParallelOptions options) |
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.
ported these two helpers over as well. they're normally internal properties on ParallelOptions, so we don't have direct access to them anymore.
src/Workspaces/Core/Portable/Shared/Utilities/RoslynParallel.NetFramework.cs
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Shared/Utilities/RoslynParallel.NetFramework.cs
Show resolved
Hide resolved
} | ||
|
||
#if false | ||
/// <summary>Executes the task body using the <see cref="ExecutionContext"/> captured when ForEachAsync was invoked.</summary> |
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.
i think this should be ok. checking with @stephentoub .
@ToddGrun this is ready for review. Will be holding off from merging this until i get OK from stephen on the changes i made. |
Ok. Looks like Stephen is good. @ToddGrun this is ready for review. |
#pragma warning disable VSTHRD200 // Use "Async" suffix for async methods | ||
|
||
// Ported from | ||
// https://github.com/dotnet/runtime/blob/main/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/Parallel.ForEachAsync.cs |
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.
to be very clear. this is effectively the runtime's code. just tweaked as little as possible to get working on netfx.
i do not want to change it beyond that, as that will be much harder to keep in sync and validate against the netcore impl.
Did a cursory review, I'm going to trust Stephen and Cyrus on this one. |
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.
@jasonmalinowski For review when you get back. |
No description provided.