CsWinRT icon indicating copy to clipboard operation
CsWinRT copied to clipboard

Make DispatcherQueueSynchronizationContext faster and zero-alloc

Open Sergio0694 opened this issue 2 years ago • 3 comments

Overview

The current DispatcherQueueSynchronizationContext implementation is rather inefficient, for two reasons:

  • It's creating a delegate with closure every single time (!), capturing both the callback and the state
  • There's also additional overhead in general due to how the actual callback is then scheduled internally

Here's the specific line that's troubling:

https://github.com/microsoft/CsWinRT/blob/31a191d0e9b557e3732f93d86cf626defbb8d009/src/Projections/Reunion/Microsoft.UI.Dispatching.DispatcherQueueSynchronizationContext.cs#L25

This type is used extremely frequently in applications, especially for those that eg. rely on portable libraries for things such as MVVM support etc. that also deal with proper dispatching. The way that works is by capturing the synchronization context during setup and then dispatch through that, meaning this Post method will get invoked every time.

The proposed solution

This PR optimizes the Post implementation by making it faster and zero-alloc. There are no public API changes. Opening this as draft as I'm not entirely sure of the procedure to add tests for this for this specific repo. Pinging @jkoritzinsky as we discussed this feature idea previously on Discord as well 😄

I run some benchmarks for this a while back, got some nice numbers:

Method Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
TryEnqueueWithClosure 7.062 us 0.1353 us 0.1852 us 1.00 0.0687 - - 312 B
TryEnqueueWithState 1.139 us 0.0229 us 0.0676 us 0.15 - - - -

In this scenario dispatching with this system was about 7x faster, and also completely allocation free. I'd argue it'd be nice to have such a basic building block like this one to be as efficient as possible.

I'm sure it might be a good idea to setup up some new ones in this repo as well, though I'd need guidance on that 😅

Opening as draft for now because of this, can switch this once everyone's happy with coverage and overall setup.

Related issues/PRs

  • https://github.com/CommunityToolkit/WindowsCommunityToolkit/pull/4097
  • https://github.com/microsoft/microsoft-ui-xaml/issues/3321

Sergio0694 avatar Nov 29 '21 17:11 Sergio0694

Thanks @Sergio0694 for this contribution. A ~7x improvement with reduced memory pressure is definitely great to see in something that can be frequently used. The version of this type that you have edited here is actually a copy of the one in WinAppSDK and is here for test purposes. Adding @jeffstall and @ionvasilian who own this type and should be able to consider and update the actual one in WinAppSDK.

I looked over the change and it looks good to me. It basically looks like you use a natively allocated proxy instance to hold onto the callback and state and make it look like a delegate by implementing Invoke and then fetch them later when invoked to make the call. The only thing I wonder is if the delegate proxy should also implement IReferenceTrackerTarget. I am not sure if XAML reference tracking is used for this type or not, but Jeff or Ion might be able to comment on that.

manodasanW avatar Dec 01 '21 03:12 manodasanW

Hi @manodasanW, thank you for taking a look, really glad to hear this is something that could be interesting to have! 🎉 I wasn't aware this type was just a copy and not the real one (whoops!), but then again the main point of this PR was to show a proof of concept, share some numbers and start a conversation about this. Happy to help migrate this if needed! 😄

"I looked over the change and it looks good to me. It basically looks like you use a natively allocated proxy instance to hold onto the callback and state and make it look like a delegate by implementing Invoke and then fetch them later when invoked to make the call."

Yup, that's correct! The underlying WinRT API for IDispatcherQueue::TryEnqueue is just this one:

int TryEnqueue(IDispatcherQueueHandler* callback, byte* result);

Where that callback is this an object inheriting from IDispatcherQueueHandler, which is:

class IDispatcherQueueHandler : IUnknown
{
    int Invoke();
}

So in this PR I'm just creating my own implementation instead of letting CsWinRT create one wrapping that managed delegate/closure (we're essentially defining a C# struct that implements an object derived from IDispatcherQueueHandler). This means I can then also make it blittable and just store a couple GC handles in it to track the managed objects to invoke later (delegate and optional state), without extra allocations. And the Invoke() method which is then invoked by the WinRT runtime when the callback is dispatched simply grabs the target callback and state back and invokes them, and then ensures exceptions are tracked correctly, if any. That IReferenceTrackerTarget interface hasn't come up while investigating this so far (as in, IDispatcherQueueHandler didn't inherit from it, and when testing this and checking with a breakpoint on QueryInterface, that interface was never queried), but for sure, waiting for Jeff to confirm this or suggest any changes 😊

Sergio0694 avatar Dec 01 '21 10:12 Sergio0694

Publishing this PR as the issue raised by @jkotas has been resolved, and there doesn't seem to be other outstanding ones. Chatted with @jkoritzinsky on Discord and he said it seems all obvious concerns have been addressed. @manodasanW would these changes still also need to be merged here to mirror the actual type in WASDK? @jeffstall, @ionvasilian happy to help eventually migrating these changes to the right place in WASDK if these changes are accepted 😊

Sergio0694 avatar Jan 11 '22 21:01 Sergio0694

Any updates on this?

AtariDreams avatar Oct 20 '22 15:10 AtariDreams

We ultimately decided not to port these changes into WinAppSDK. They add considerable code complexity, and we could not justify the added risk.


In reply to: 1285755982

benwestprog avatar Oct 20 '22 16:10 benwestprog

Thanks @Sergio0694 for trying anyway. It would have been nice to have!

(Too bad GH doesn't have a sad-face emoji to express sadness without disapproval. A 👎 or 😕do not convey the same meaning!)

mqudsi avatar Oct 20 '22 17:10 mqudsi

@mqudsi Thank you! 😊

For the record, we've been using this implementation in the Microsoft Store for months already (just slightly different, as the Store uses UWP and .NET Native, not CsWinRT, but it's effectively identical minus a different GUID), and it's something I do want us to open source eventually. If CsWinRT is not the right path, we might look into open sourcing it in the Windows Community Toolkit at some point, maybe packaged in some higher-level API (like the dispatcher service we showed at //BUILD 2022) that people could use like we're doing, we'll see. Of course, you can also just copy this implementation for yourself in the meantime 🙂

Sergio0694 avatar Oct 20 '22 17:10 Sergio0694