System.Linq.Parallel OrderedPipeliningMergeEnumerator.MoveNext is using API unsupported on browser

[buyaa-n]

System.Linq.Parallel OrderedPipeliningMergeEnumerator.MoveNext is using API unsupported on browser (Monitor.Wait();). It needs to be fixed so it works without Monitor.Wait() on browser

OrderedPipeliningMergeEnumerator.TryWaitForElement It is calling Monitor.Wait() which is unsupported on browser. Which is further called from OrderedPipeliningMergeEnumerator.MoveNext which is overriding IEnumerator.MoveNext(...)

Originally posted by @buyaa-n in #43363 (comment)

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

[ghost]

Tagging subscribers to this area: @tarekgh
See info in area-owners.md if you want to be subscribed.

[buyaa-n]

AsynchronousChannelMergeEnumerator.MoveNextSlowPath() also accessing non suppurted ManualResetEventSlim.Wait(), i think it doesn't need separate issue as AsynchronousChannelMergeEnumerator.MoveNextSlowPath() also affecting IEnumerator.MoveNext(...)

[buyaa-n]

More similar references found:

1. OrderedHashRepartitionEnumerator.MoveNext(ref Pair<TInputOutput, THashKey> currentElement, ref TOrderKey currentKey)
2. HashRepartitionEnumerator.MoveNext(ref Pair<TInputOutput, THashKey> currentElement, ref int currentKey)
3. DefaultIfEmptyQueryOperator.MoveNext(ref TSource currentElement, ref TKey currentKey)
4. FirstQueryOperator.MoveNext(ref TSource currentElement, ref int currentKey)
5. LastQueryOperator.MoveNext(ref TSource currentElement, ref int currentKey)
6. TakeOrSkipQueryOperator.MoveNext(ref TResult currentElement, ref TKey currentKey)
7. TakeOrSkipWhileQueryOperator. MoveNext(ref TResult currentElement, ref TKey currentKey)

[stephentoub]

These are all the guts of PLINQ. Really it's just that PLINQ isn't supported on browser, all up. Either the whole assembly should be annotated as unsupported, or if there's a goal of having it be supported, browser should have its own effectively nop implementation that just delegates everything to Enumerable.

[buyaa-n]

As per the suggestion and discussion, marked the entire assembly unsupported

[akoeplinger]

I think we should do what @stephentoub suggested and replace it with an implementation that delegates to Enumerable.

[danmoseley]

@akoeplinger does your team expect to do this by Aug 17th

[akoeplinger]

@lewing @steveisok what do you think?

[kg]

I started working on this but it's not clear to me whether this work is actually valuable. If I enable the PLINQ tests on the existing codebase (i.e. main), about 27000 of them run and pass successfully. With my new explicit implementation (all the PLINQ methods manually delegated to regular LINQ) I get over 2000 test failures due to things like differences in exception handling and bugs in the tests.

Are we sure PLINQ doesn't actually mostly work in the browser as-is? Do we have applications that are broken that I can look into and identify fixes for? I don't think replacing PLINQ wholesale is actually the right solution unless we really need to do it, it makes the code incredibly messy.

[akoeplinger]

I guess we never hit the case of partition.Count > 1 here on Browser:

runtime/src/libraries/System.Linq.Parallel/src/System/Linq/Parallel/Merging/MergeExecutor.cs

Lines 65 to 77 in 8bfb45a

	if (partitions.PartitionCount > 1)
	{
	// We use a pipelining ordered merge
	mergeExecutor._mergeHelper = new OrderPreservingPipeliningMergeHelper<TInputOutput, TKey>(
	partitions, taskScheduler, cancellationState, autoBuffered, queryId, partitions.KeyComparer);
	}
	else
	{
	// When DOP=1, the default merge simply returns the single producer enumerator to the consumer. This way, ordering
	// does not add any extra overhead, and no producer task needs to be scheduled.
	mergeExecutor._mergeHelper = new DefaultMergeHelper<TInputOutput, TKey>(
	partitions, false, options, taskScheduler, cancellationState, queryId);
	}

So the Monitor.Wait() in OrderedPipeliningMergeEnumerator is likely never hit and we could suppress the analyzer warning.

[kg]

I guess we never hit the case of partition.Count > 1 here on Browser:

runtime/src/libraries/System.Linq.Parallel/src/System/Linq/Parallel/Merging/MergeExecutor.cs

Lines 65 to 77 in 8bfb45a

	if (partitions.PartitionCount > 1)
	{
	// We use a pipelining ordered merge
	mergeExecutor._mergeHelper = new OrderPreservingPipeliningMergeHelper<TInputOutput, TKey>(
	partitions, taskScheduler, cancellationState, autoBuffered, queryId, partitions.KeyComparer);
	}
	else
	{
	// When DOP=1, the default merge simply returns the single producer enumerator to the consumer. This way, ordering
	// does not add any extra overhead, and no producer task needs to be scheduled.
	mergeExecutor._mergeHelper = new DefaultMergeHelper<TInputOutput, TKey>(
	partitions, false, options, taskScheduler, cancellationState, queryId);
	}

So the Monitor.Wait() in OrderedPipeliningMergeEnumerator is likely never hit and we could suppress the analyzer warning.

That makes sense. I think I'll make a new branch and try just chasing down any place where it might try to fork threads and see how many of those need modification to ensure they never will.

I expect there's code out there that will do things like try to use synchronization primitives inside select lambdas or something, but that can't possibly work no matter what I do.

[akoeplinger]

You can remove this from the Directory.Build.props and it should tell you about methods that are unsupported on Browser:

runtime/src/libraries/System.Linq.Parallel/Directory.Build.props

Line 5 in 8bfb45a

<UnsupportedOSPlatforms>browser</UnsupportedOSPlatforms>

[kg]

Yeah, my branch has that removed and then I chased down all the analyzer warnings (there were something like a thousand of them) - right now my branch builds with no warnings.

[kg]

#58227 suppresses all the analyzer warnings and all the tests still pass, so that might be all that's necessary here? I would love to know if there are any developers who've contacted us to ask for PLINQ.