dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.93k stars 4.64k forks source link

[API Proposal]: Span overload for SynchronizationContext.Wait(...) #104858

Open h3xds1nz opened 1 month ago

h3xds1nz commented 1 month ago

Background and motivation

Currently, SynchronizationContext has a public virtual int Wait(...) method that takes array an of IntPtr and a protected WaitHelper method which also takes an array of IntPtr, which is by default called by the public member.

One of the frequent call chains originate from WaitOne->WaitOneNoCheck from WaitHandle class and its derivations, which is currently always forced to allocate a heap-based array of a single IntPtr, causing unnecessary GC overhead.

Since the protected WaitHelper helper from SynchronizationContext calls into WaitHandle.WaitMultipleIgnoringSyncContext(...) which already takes a ReadOnlySpan<IntPtr>, by adding public ReadOnlySpan<IntPtr> overloads onto SynchronizationContext we could get rid of this unnecessary allocation when awaiting a single handle in the runtime code, and also in user-code.

API Proposal

namespace System.Threading;

public partial class SynchronizationContext
{
+    public virtual int Wait(bool waitAll, int millisecondsTimeout, params ReadOnlySpan<IntPtr> waitHandles);
+    protected static int WaitHelper(bool waitAll, int millisecondsTimeout, params ReadOnlySpan<IntPtr> waitHandles);
}

API Usage

namespace System.Threading;

public partial class WaitHandle
{
    private bool WaitOneNoCheck(int millisecondsTimeout)
    {
        // some code
        int num = current.Wait(false, millisecondsTimeout, waitHandle.DangerousGetHandle());
        // some code
    }
}
dotnet-policy-service[bot] commented 1 month ago

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

gfoidl commented 1 month ago

Taking latest language features into account the proposal could be

namespace System.Threading;

public partial class SynchronizationContext
{
+    public virtual int Wait(params ReadOnlySpan<IntPtr> waitHandles, bool waitAll, int millisecondsTimeout);
+    protected static int WaitHelper(params ReadOnlySpan<IntPtr> waitHandles, bool waitAll, int millisecondsTimeout);
}

But as given above this wouldn't compile, as the params parameter must be last, so actually it could be

namespace System.Threading;

public partial class SynchronizationContext
{
+    public virtual int Wait(bool waitAll, int millisecondsTimeout, params ReadOnlySpan<IntPtr> waitHandles);
+    protected static int WaitHelper(bool waitAll, int millisecondsTimeout, params ReadOnlySpan<IntPtr> waitHandles);
}
stephentoub commented 1 month ago

Can you elaborate on why this particular allocation is impactful to you? What SC are you using where you've asked for wait notification and what's the calling pattern where you're frequently doing blocking waits on threads with this context current?

h3xds1nz commented 1 month ago

@gfoidl The second proposal could do, this would also aid to the fact with Span<T> overloads being preferred over T[] and since those methods are virtual which I didn't consider before.

However, since the underlying WaitHandle methods use Span<IntPtr>, you would be unable to call those when we use ReadOnlySpan<IntPtr> unless the chain is adjusted, unsure whether it is modified anywhere. That is why I didn't initially go with ROS.

@stephentoub Thank you for your interest on this. I originally came onto this alloc when looking at total GC time. The most usual chain here for me were Dispatcher Invokes in WPF and the underlying operations, queue posts from long-running background threads to update UI, which uses ManualResetEvent for DispatcherOperationEvent.

gfoidl commented 1 month ago

However, since the underlying WaitHandle methods use Span<IntPtr>, you would be unable to call those when we use ReadOnlySpan<IntPtr> unless the chain is adjusted, unsure whether it is modified anywhere.

This gets addressed in https://github.com/dotnet/runtime/pull/104864.

h3xds1nz commented 1 month ago

This gets addressed in #104864.

Amazing, thanks for that, I have updated the proposal based on your suggestion.

rickbrew commented 3 weeks ago

Can you elaborate on why this particular allocation is impactful to you? What SC are you using where you've asked for wait notification and what's the calling pattern where you're frequently doing blocking waits on threads with this context current?

Here's an example of how I'm using this (not publicly accessible link) : https://github.com/dotnet/paint.net/blob/bc539c00f25e9f5bcafb4146cfc449532cfb29b9/Windows.Framework/Threading/TimerThread.cs#L250

The use here is clumsy, but not performance impacting. I'd love to see this become less clumsy, but it's not a high priority for my uses.

Because there's no span-based versions of these APIs, I have to use new WaitHandle[]. I cannot use ArrayPool<WaitHandle>.Shared.Rent() and then slice the span to the appropriate length. In my case I only need to allocate once at app startup, so it's no big deal, but any kind of high-traffic situation becomes more clumsy as you need to implement your own local array pooling.

rickbrew commented 3 weeks ago

The code is essentially this. So I can't use the array pool, and it's necessary to allocate two arrays for the two waits that use 3 and 2 handles respectively.

This is for WaitHandle.Wait[Any|All]() but it's the same idea as SynchronizationContext.Wait().

private void TimerThreadProc()
{
    WaitHandle[] waitHandles3 = new WaitHandle[3];
    waitHandles3[0] = this.isCancelledEvent;
    waitHandles3[1] = this.isEnabledEvent;
    waitHandles3[2] = this.pulseEvent;

    WaitHandle[] waitHandles2 = new WaitHandle[2];
    waitHandles2[0] = this.isCancelledEvent;
    waitHandles2[2] = this.waitableTimer; // Win32 waitable timer

    while (true)
    {
         // wait on the 3 handles array
         int waitIndex = WaitHandle.WaitAny(waitHandles3);
         ...
         // wait on the 2 handles array
         waitIndex = WaitHandle.WaitAny(waitHandles2);
         ...
    }
}