Cleanup unmaintainable parallel logic with workers #77993
Conversation
* removed line breaks
…r and their associated create worker methods
* Created joint functions for all For 32 vs 64 entry points
…eanup-unmaintainable-parallel-logic-with-workers # Conflicts: # src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/Parallel.cs
Now going to work on migrating foreach to new imp
Removed unnecessary object initializations from entry points after factory simplification.
…ossible. moved Common code from orderable and partitioner foreach implementation - prep for merge with For worker. Fixed line breaks
ghost
added
area-System.Threading
community-contribution
|
Tagging subscribers to this area: @mangod9 Issue Detailsnull
|
|
Tagging subscribers to this area: @dotnet/area-system-threading-tasks Issue Detailsnull
|
| internal sealed class Box<T> where T: class | ||
| { | ||
| public T? Value { get; set; } | ||
| } | ||
|
|
There was a problem hiding this comment.
How about you use System.Runtime.CompilerServices.StrongBox<T> instead?
There was a problem hiding this comment.
Cool! didn't know it existed - I assumed there had to be one already implemented.
|
cc: @stephentoub |
| IWorkerBody<TValue> CreateWorkerBody(ParallelLoopStateFlags sharedPStateFlags); | ||
| } | ||
|
|
||
| internal sealed class WorkerWithSimpleBodyFactory<TValue>: IWorkerBodyFactory<TValue> |
There was a problem hiding this comment.
I can change all the factories to structs if that is desirable - there is currently no inheritance.
This means I could change the functions receiving them to be generic over the factory and (theoretically) reduce allocation.
Should I do this?
| } | ||
| } | ||
|
|
||
| internal abstract class WorkerBodyWithIndex<TValue, TIndex> : IWorkerBodyWithIndex<TValue, TIndex> |
There was a problem hiding this comment.
with a little code duplication in the worker classes I can change them from classes to structs and reduce allocations if that is desirable.
Thoughts?
|
|
||
| #region Workers | ||
|
|
||
| internal interface IWorkerFactory<in TSource, TInput, TValue, out TIndex> where TIndex: INumber<TIndex> |
There was a problem hiding this comment.
This interface started as a factory only interface but now contains more loop run related metadata.
Is there a better name for this?
|
Is there an automatic formatter that complies with the runtime repo guidelines? |
|
Where and what docs should I add in the code? |
This is a superset. |
|
Sounds good, thanks for the details. Expect a deeper review of both PRs in early January, and feel free to ping me if this falls through the cracks. |
|
@dakersnar Hi! any news? I didn't remined earlier since I was busy. |
|
Thanks for the reminder, and sorry for the delay. This is on my agenda; I'll take a look early next week. |
|
@tjwald, thank you for trying to tackle this, and apologies for taking so long to getting around to looking at it. I very much appreciate your work here and your interest in contributing. That said, I'm concerned about a couple of things. First and foremost (and having absolutely nothing to do with your work here), there is a lot of code involved here that's been working fine for a decade. The issue that was opened about consolidating logic wasn't a request from the .NET team but rather a comment from someone in the community that it could be cleaned up. And while it's certainly nice to try to consolidate, to ensure there aren't any regressions we'd need to go through the changes line-by-line with a fine-toothed comb to confirm that semantics are being maintained 100%. With the long-term maintenance responsibility on our shoulders, this is one of those times where it's a lot harder for us to accept an external contribution, because the time it takes to review/compare line-by-line and validate that nothing has gone wrong accidentally is a lot more than the time it takes to just do the work. Second, I see in the changes that new instance interface methods and new instance virtual methods are being introduced; that's an example of the kind of change that can have a negative performance implication rather than functional regressions like I was concerned about in my first point. Third, from just a quick skim I do see places where there are functional problems, e.g. making the ETW provider generic and leading to different event signatures based on which caller is being used. This issue shouldn't have been marked as "help wanted". That's our bad, and I apologize for that and that it's ended up wasting your time. I'm going to close this PR and try my hand at doing the consolidation myself. I'll then compare what I came up with with what you have here... if they end up being close enough, I'll cherry-pick some of your commits. Either way, I can tag you when I open a PR for it in case you'd like to review. Thank you, again, and apologies, again. If you'd like some help choosing a better issue on which to contribute, I'd be happy to help. |
|
@stephentoub I perfectly understand, and I am happy that the minimal change was done and merged! |
ghost
locked as resolved and limited conversation to collaborators
6 participants
Relates to Issue
This PR is a continuation of the previous PR
This PR uses worker objects and worker bodies to unify the implementation of 3 out of the 4 core logics in Parallel:
For32For64Foreach(Both Orderable and Partitioner)The only one that is not covered is
Invokethat had a completely separate implementation.