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

Dispatcher helper optimizations #3119

Conversation

Sergio0694
Copy link
Member

This PR is part of the group being tracked in #3108.

PR Type

What kind of change does this PR introduce?

  • Optimization

What is the current behavior?

Currently, APIs in the DispatcherHelper class will always result in a callback dispatch on the target CoreDispatcher instance, and a Task or Task<T> allocation, even when the caller is already running on that same CoreDispatcher instance, therefore not needing the dispatching at all.

What is the new behavior?

There are just 2 changes being introduced by this PR:

  • When scheduling an Action or Func<T> on the UI thread, if the target CoreDispatcher already has thread access, the delegates will be invoked directly, skipping the dispatcher scheduling entirely.
  • When running an Action on the same UI thread, Task.CompletedTask is used, to avoid the unnecessary allocation of a new Task instance every time.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

@ghost
Copy link

ghost commented Feb 1, 2020

Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

vgromfeld
vgromfeld previously approved these changes Feb 3, 2020
@michael-hawker
Copy link
Member

@Sergio0694 did a few things just shuffle around in the order of the file, saw some signature changes, but just wanted to quickly check the public API surface was all the same in the end?

Happy to change it, but we'd need to move to the 7.0 dev branch for anything that breaks the API.

I'll look at this more closely a bit later, was just taking a quick peak first.

@Sergio0694
Copy link
Member Author

@michael-hawker I didn't make any signature changes actually (or I would've marked the PR as being a breaking change), I just reordered the methods to be more structured. The previous order they were using in the file was just making it harder to follow the program flow I think.

The only real change in this PR is the improved efficiency by skipping the dispatcher scheduling when the invoking thread is the same one and already has UI thread access 😄

@michael-hawker
Copy link
Member

Thanks @Sergio0694, I went through commit-by-commit, thanks for separating the re-order as it's own commit :)

Would it be good for us to add some Unit Tests to these functions?

@JohnnyWestlake
Copy link
Contributor

@michael-hawker I didn't make any signature changes actually (or I would've marked the PR as being a breaking change), I just reordered the methods to be more structured. The previous order they were using in the file was just making it harder to follow the program flow I think.

The only real change in this PR is the improved efficiency by skipping the dispatcher scheduling when the invoking thread is the same one and already has UI thread access 😄

Fwiw, this can be a break change depending on how the developer has written their apps. It's making what was an asynchronous call now essentially synchronous, which is changing the order things are executed in which can lead to a lot of unintended - and potentially difficult to track down - side effects.

Give the default Dispatcher.RunAsync will always reschedule and not run immediately it is probably safer to stick with that default behaviour, and potentially add another method to DispatcherHelper that checks first.

@Sergio0694
Copy link
Member Author

@JohnnyWestlake Ah, yeah that's fair. I guess what I meant by "no breaking changes" was the fact that the current behavior was entirely non deterministic (since dispatched callbacks could be scheduled at any point in time that's not known in advance), so that shouldn't have been something developers would be relying upon. And by that rationale, changing that so that some calls might sometimes be invoked synchronously, would make no difference, conceptually - it'd still be some point in time after the method is invoked, and in some cases that point in time would just end up happening a tiny bit sooner.

That said, I understand your point. If you'd like me to separate those methods through a different API (though I'd argue that'd make the API surface a bit crowded and possibly harder to understand for consumers), do you have a name for that in mind? 🤔

@michael-hawker michael-hawker added this to the 7.0 milestone Mar 24, 2020
@michael-hawker
Copy link
Member

@Sergio0694 this is a cool addition to these helpers. I agree with @JohnnyWestlake that while the API is the same, changing this behavior could adversely effect some potential code-paths (unknowingly). I think it'd be good to call out as a perf improvement, but with a note for developers using it as a 'breaking change' (for behavior) in our 7.0 release first.

Would you mind rebasing this change on top of the dev/7.0.0 branch and potentially adding some Unit Tests in general for these dispatchers to help us protect their functionality (and you new additions) in the future?

@michael-hawker michael-hawker changed the base branch from master to dev/7.0.0 March 24, 2020 21:14
@Sergio0694 Sergio0694 force-pushed the optimization/dispatcher-helper branch from 9d510f0 to 1cebda0 Compare March 24, 2020 21:20
@ghost
Copy link

ghost commented Mar 24, 2020

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@Sergio0694
Copy link
Member Author

Sergio0694 commented Mar 24, 2020

@michael-hawker Since we've moved this PR to 7.0, I might as well throw in a couple other ideas:

  • The naming of the two APIs are a bit confusing. Right now they're ExecuteOnUIThreadAsync and AwaitableRunAsync. I find that confusing for primarily two reasons:
    1. Why is the first called Execute___ and the second Run___? They're doing the same thing, the only difference is whether they're an extension method. What about just using Run___ in all cases? The first method would simply become RunOnUIThreadAsync.
    2. Why is the second method called Awaitable___? By convention, all methods ending with "Async" return a Task-like type, which is alwais awaitable. Basically that's a given considering the suffix, so that "Awaitable" part is redundant, what about just removing it entirely?
  • Given that the non-async methods will not support running entirely in a synchronous way if the caller already has thread access, what about taking a dependency on System.Threading.Tasks.Extensions and changing the return type of the overloads taking Action and Func<T> to ValueTask and ValueTask<T>? That could save some memory allocations when the call is synchronous. Bonus point: once UWP moves to .NET 5 that package won't even be necessary at all, since that type is built-in even in just .NET Core 3.0.

Let me know what you think! 😊

@michael-hawker
Copy link
Member

@Sergio0694 those are some interesting discussion points. For the naming there's a CoreDispatcher.RunAsync method, so I believe we were trying to differentiate it from that, would something like RunAsyncAsTask make more sense or again muddle the terms? But yeah, changing Execute to Run for consistency makes sense too.

@vgromfeld @azchohfi thoughts on optimizing this a bit since we're calling it a breaking change anyway?

@Sergio0694
Copy link
Member Author

Sergio0694 commented Mar 27, 2020

@michael-hawker I think that RunAsyncAsTask would have the same issue as Awaitable___, plus that'd also go against the usual pattern of naming Task-returning methods with the "Async" suffix. Personally I don't see any problems with using RunAsync for the extension methods, they'd still be different from the instance method of the CoreDispatcher class for two reasons:

  • Different arguments, so the compiler would transparently pick the right one (the one from the toolkit) when users don't pass the dispatcher priority, or if they pass some other delegate type that's only supported by our extensions (eg. Func<T>)
  • The fact that they're marked as extension methods in the IntelliSense popup

If nothing else, I feel like having those methods using the same name would actually make them feel more "integrated" into the native APIs, which could also increase the discoverability for eg. developers just getting started on UWP (which in that case wouldn't be using the overload with the explicit dispatching priority anyway) so that'd be nice 😄

EDIT: on second thought, I'll do you one better, personally I'd rename all the methods in the DispatcherHelper class to just RunAsync. This is the same setup I had in my UICompositionAnimations library (here and here). The distinction is given by the fact that half the methods are instance and half are extension methods, and then the exact arguments. The OnUIThread part is redundant as well after all, since the class itself is called DispatcherHelper, and it's in a UI-related namespace.

@vgromfeld vgromfeld dismissed their stale review March 30, 2020 07:01

revoking review

Copy link
Contributor

@vgromfeld vgromfeld left a comment

Choose a reason for hiding this comment

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

:shipit:

@michael-hawker michael-hawker merged commit d653e55 into CommunityToolkit:dev/7.0.0 Mar 30, 2020
@Sergio0694 Sergio0694 deleted the optimization/dispatcher-helper branch March 30, 2020 21:39
@michael-hawker
Copy link
Member

Alex and I figured it makes sense to merge this PR as is now, and we can open another issue later if we want to rename them before the 7.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants