Repository navigation
Add dedicated wasm implementation for Parallel.Invoke and Parallel.For - #57283
Conversation
kg
added
NO-MERGE
|
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsThis PR introduces a simple single-threaded implementation of I still need to verify whether the other Parallel primitives need changes to properly use this in all scenarios, but it works good for the two methods I targeted right now. I was unable to get platform conditionals to work in any fashion (ifdef, etc) so it's hardcoded to on in this PR right now, which is obviously wrong. If some reviewer can figure out how the heck to make this work I'd appreciate it (see the FIXME in Parallel.cs)
|
kg
removed
the
NO-MERGE
|
Tests for these in
I think the conditional attributes can be changed to: |
|
Those tests seem to work now so I enabled them. |
|
runtime (Libraries Test Run checked coreclr OSX x64 Debug) failure is https://github.com/dotnet/core-eng/issues/14085 |
|
@stephentoub how do you feel about the platform check here? Is it better for me to try and use an ifdef or some sort of msbuild conditional? Larry pointed out that we have some similar stuff sprinkled around the threadpool but none of it is quite right atm, which makes me wonder whether we should try to centralize or rearrange things so that the check doesn't need to be a direct |
|
@kg, thanks for asking. Honestly, my preference longer-term (assuming longer-term we're still serialized for wasm) is to have a dedicated implementation for browser that just does everything serially, without all the extra hoopla, task queueing, etc. It should end up being way less code to handle. Assuming you agree that's desirable for .NET 7, I'm fine for .NET 6 in the name of getting something in and working to include the platform check here. It'll evaporate at JIT time for other workloads, and it'll be temporary until we get the real solution in place. Same goes for PLINQ. I'd just ask that we open an issue for the rewrite and include a TODO link for it in the source. |
|
That sounds fine, and I think transitioning from this to a custom small single-threaded rewrite won't be too bad. I'll file the tracking issue when we merge this. I was looking into PLINQ and it probably makes sense to treat the rewrite of this as a part of that effort. |
|
|
||
| action(ref state, timeout, out bool yieldedBeforeCompletion); | ||
| if (yieldedBeforeCompletion) | ||
| throw new Exception("Replicated tasks cannot yield in this single-threaded browser environment"); |
There was a problem hiding this comment.
Why is it not PNSE exception?
ghost
locked as resolved and limited conversation to collaborators
This PR introduces a simple single-threaded implementation of
TaskReplicatorand then makes sure thatParallel.Invokeand.Foruse it . This works around various issues like #44605, #43411.I still need to verify whether the other Parallel primitives need changes to properly use this in all scenarios, but it works good for the two methods I targeted right now.
I was unable to get platform conditionals to work in any fashion (ifdef, etc) so it's hardcoded to on in this PR right now, which is obviously wrong. If some reviewer can figure out how the heck to make this work I'd appreciate it (see the FIXME in Parallel.cs)