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 dedicated wasm implementation for Parallel.Invoke and Parallel.For #57283

Merged
merged 8 commits into from
Aug 17, 2021

Conversation

kg
Copy link
Member

@kg kg commented Aug 12, 2021

This PR introduces a simple single-threaded implementation of TaskReplicator and then makes sure that Parallel.Invoke and .For use 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)

@kg kg added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) arch-wasm WebAssembly architecture labels Aug 12, 2021
@ghost
Copy link

ghost commented Aug 12, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR introduces a simple single-threaded implementation of TaskReplicator and then makes sure that Parallel.Invoke and .For use 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)

Author: kg
Assignees: -
Labels:

* NO MERGE *, arch-wasm

Milestone: -

@kg kg requested review from stephentoub and lewing August 12, 2021 13:10
@adamsitnik adamsitnik added this to the 6.0.0 milestone Aug 12, 2021
@kg kg removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 13, 2021
@radical
Copy link
Member

radical commented Aug 13, 2021

Tests for these in src/libraries/System.Threading.Tasks.Parallel/tests, like ParallelFor.cs, BreakTests.cs are currently disabled as they use PlatformDetection.IsThreadingSupported. They need to be enabled.

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs: public static bool IsThreadingSupported => !IsBrowser;

I think the conditional attributes can be changed to:
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported), nameof(PlatformDetection.IsNotBrowser)], if we want to retain the IsThreadingSupported, else, just remove them.

@kg
Copy link
Member Author

kg commented Aug 17, 2021

Those tests seem to work now so I enabled them.

@lewing
Copy link
Member

lewing commented Aug 17, 2021

runtime (Libraries Test Run checked coreclr OSX x64 Debug) failure is https://github.com/dotnet/core-eng/issues/14085

@kg
Copy link
Member Author

kg commented Aug 17, 2021

@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 OperatingSystem.IsBrowser() here.

@stephentoub
Copy link
Member

@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.

@kg
Copy link
Member Author

kg commented Aug 17, 2021

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");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it not PNSE exception?

@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants