Proposal - Replace internal use of MessagingCenter by hartez · Pull Request #3880 · dotnet/maui

hartez · 2021-12-29T23:04:41Z

This is related to #3797 (and also to #95).

This is a draft of what it would look like to utilize the Community Toolkit Messenger for the messaging internals in Controls.

This is mostly just a quick-and-dirty direct replacement. There are some places where the features of Messenger could be better utilized for more performance (e.g., in several places we could update the message handlers to use static methods).

This is intended as a discussion starting point / proof-of-concept.

Sergio0694

Love where this is going, thank you for putting together this proposal! 🚀
I've left some quick low hanging improvements that could be added 😄

Sergio0694 · 2021-12-29T23:22:26Z

src/Compatibility/Core/src/Android/AppCompat/Platform.cs

 				_navAnimationInProgress = value;
 				if (value)
-					MessagingCenter.Send(this, CloseContextActionsSignalName);
+					WeakReferenceMessenger.Default.Send(new CloseContextActionsMessage());


Nit: this could be simplied through the extension:

Suggested change

WeakReferenceMessenger.Default.Send(new CloseContextActionsMessage());

WeakReferenceMessenger.Default.Send<CloseContextActionsMessage>();

Sergio0694 · 2021-12-29T23:25:33Z

src/Compatibility/Core/src/Android/PopupManager.cs

+				WeakReferenceMessenger.Default.Register<Activity, PageBusyMessage>(Activity, (r, m) => OnPageBusy(m.Page, m.IsBusy));
+				WeakReferenceMessenger.Default.Register<Activity, PageAlertMessage>(Activity, (r, m) => OnAlertRequested(m.Page, m.Arguments));
+				WeakReferenceMessenger.Default.Register<Activity, PromptMessage>(Activity, (r, m) => OnPromptRequested(m.Page, m.Arguments));
+				WeakReferenceMessenger.Default.Register<Activity, ActionSheetMessage>(Activity, (r, m) => OnActionSheetRequested(m.Page, m.Arguments));


Nit: if we don't care about the strongly-typed recipient (it's not used in the handler), this can be simplified.
Additionally (though up to style preference), the recipient can be a discard to make it more explicit:

Suggested change

WeakReferenceMessenger.Default.Register<Activity, PageBusyMessage>(Activity, (r, m) => OnPageBusy(m.Page, m.IsBusy));

WeakReferenceMessenger.Default.Register<Activity, PageAlertMessage>(Activity, (r, m) => OnAlertRequested(m.Page, m.Arguments));

WeakReferenceMessenger.Default.Register<Activity, PromptMessage>(Activity, (r, m) => OnPromptRequested(m.Page, m.Arguments));

WeakReferenceMessenger.Default.Register<Activity, ActionSheetMessage>(Activity, (r, m) => OnActionSheetRequested(m.Page, m.Arguments));

WeakReferenceMessenger.Default.Register<PageBusyMessage>(Activity, (_, m) => OnPageBusy(m.Page, m.IsBusy));

WeakReferenceMessenger.Default.Register<PageAlertMessage>(Activity, (_, m) => OnAlertRequested(m.Page, m.Arguments));

WeakReferenceMessenger.Default.Register<PromptMessage>(Activity, (_, m) => OnPromptRequested(m.Page, m.Arguments));

WeakReferenceMessenger.Default.Register<ActionSheetMessage>(Activity, (_, m) => OnActionSheetRequested(m.Page, m.Arguments));

Sergio0694 · 2021-12-29T23:28:27Z

src/Compatibility/Core/src/Android/Renderers/ListViewAdapter.cs

 			realListView.OnItemLongClickListener = this;

-			MessagingCenter.Subscribe<ListViewAdapter>(this, Platform.CloseContextActionsSignalName, lva => CloseContextActions());
+			WeakReferenceMessenger.Default.Register<CloseContextActionsMessage>(this, (recipient, msg) => CloseContextActions());


This is introducing a closure, we can skip the allocation by invoking directly on the recipient:

Suggested change

WeakReferenceMessenger.Default.Register<CloseContextActionsMessage>(this, (recipient, msg) => CloseContextActions());

WeakReferenceMessenger.Default.Register<ListViewAdapter, CloseContextActionsMessage>(this, (r, _) => r.CloseContextActions());

Sergio0694 · 2021-12-29T23:30:27Z

src/Compatibility/Core/src/Windows/Platform.cs

 			_container.SizeChanged += OnRendererSizeChanged;

-			MessagingCenter.Subscribe(this, Page.BusySetSignalName, (Page sender, bool enabled) =>
+			WeakReferenceMessenger.Default.Register<UI.Xaml.Window, PageBusyMessage>(page, (window, message) =>


The recipient is not used, we can drop the first type argument:

Suggested change

WeakReferenceMessenger.Default.Register<UI.Xaml.Window, PageBusyMessage>(page, (window, message) =>

WeakReferenceMessenger.Default.Register<PageBusyMessage>(page, (_, message) =>

Sergio0694 · 2021-12-29T23:35:19Z

src/Compatibility/Core/src/iOS/Platform.cs

 			var busyCount = 0;
-			MessagingCenter.Subscribe(this, Page.BusySetSignalName, (Page sender, bool enabled) =>
+
+			WeakReferenceMessenger.Default.Register<Platform, PageBusyMessage>(this, (platform, message) =>


This is introducing a closure to capture this in the handler, as it's calling that PageIsChildOfPlatform. Why not invoke it from the recipient directly instead to avoid allocations? Same thing for the other 3 handlers defined below as well.

Sergio0694 · 2021-12-29T23:40:41Z

src/Compatibility/Maps/src/iOS/MapRenderer.cs

 				}

-				MessagingCenter.Subscribe<Map, MapSpan>(this, MoveMessageName, (s, a) => MoveToRegion(a), mapModel);
+				WeakReferenceMessenger.Default.Register<MapRenderer, MapSpan>(this, (renderer, args) => MoveToRegion(args));


Same here with the closure:

Suggested change

WeakReferenceMessenger.Default.Register<MapRenderer, MapSpan>(this, (renderer, args) => MoveToRegion(args));

WeakReferenceMessenger.Default.Register<MapRenderer, MapSpan>(this, (r, args) => r.MoveToRegion(args));

Additionally, all these handles could be static, for clarity and to avoid mistakes.

src/Controls/src/Core/Page.cs

src/Controls/src/Core/Platform/AlertManager/AlertManager.Android.cs

Sergio0694 · 2021-12-29T23:45:09Z

src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.Android.cs

 				_navAnimationInProgress = value;
 				if (value)
-					MessagingCenter.Send(this, CloseContextActionsSignalName);
+					WeakReferenceMessenger.Default.Send(new CloseContextActionsMessage());


Nit: could be simplified:

Suggested change

WeakReferenceMessenger.Default.Send(new CloseContextActionsMessage());

WeakReferenceMessenger.Default.Send<CloseContextActionsMessage>();

Cool, I didn't know that it could automatically create the message for me.
Question, though - are there performance implications here? Is this using Activator.CreateInstance to create the message object?

Sergio0694 · 2021-12-29T23:47:56Z

src/Controls/src/Core/RadioButtonGroupController.cs

-				HandleRadioButtonGroupNameChanged);
-			MessagingCenter.Subscribe<RadioButton, RadioButtonValueChanged>(this, RadioButton.ValueChangedMessage,
-				HandleRadioButtonValueChanged);
+			WeakReferenceMessenger.Default.Register<RadioButtonGroupController, RadioButtonGroupSelectionChanged>(this, HandleRadioButtonGroupSelectionChanged);


These registrations are introducing closures, and also the controller parameter in the handlers doesn't actually seem to be used at all? Those could drop that parameter, and then the handlers here could be (r, m) => r.Handle___(m), which would just cache a single delegate and avoid allocations.

The controller parameter isn't being used yet; I was hoping to convert the HandleX methods to static later (rather than having them be instance methods on RBGController).

I guess that's a question, though - which is better for performance? Instance methods (and don't pass in the controller parameter), or static methods? My assumption was that the static option was better.

The most efficient solution is one that allows the registration to be allocation free, which needs to (1) not be creating a closure, so not capturing anything, and (2) allow Roslyn to cache and reuse the delegate instance. The way the compiler works right now, the method group syntax will always result in a new delegate instance being allocated. So the optimal way to register handlers (without using IRecipient<TMessage>), would be to use an instance method being invoked directly on the input recipient. This will alow the delegate to be stateless and reusable, and there will also be no allocations due to closures 😊

Cool, I'll fix that (and change the RadioButton suff back to instance methods, too).

optimal way to register handlers (without using IRecipient)

I'd much rather use IRecipient for some of these (one RegisterAll call vs 4 for things like the AlertManager) - what are the performance implications if I go that direction?

So, there are a few considerations with using IRecipient<TMessage>, I'll try to go through all of them:

Once a handler is registered, they're mostly equivalent. Invoking a handler that was registered through IRecipient<TMessage> is actually ever so slightly faster, as there's a hot path for it in the broadcast code that allows the messenger to skip the callvirt to invoke the delegate. I wouldn't say it's a major difference though.

If you register handlers individually (that is, using IMessenger.Register<TMessage>(IRecipient<TMessage>)), the registration is free and super fast, basically the same as what you're doing now manually.

If you want to use the RegisterAll extension, that comes with some caveats. It either needs the source-generated registration methods (generated by the generator that ships with the MVVM Toolkit) to be available (meaning you either have to disable trimming, or manually add annotations to keep that generated method), or it will fallback to a path using a compiled LINQ Expressions, which will have a fair bit of overhead for the first invocation (the generated code will then be cached and reused for further invocations).

Another aspect with IRecipient<TMessage> is that it adds a public interface to the recipient type. If that type is public, that's an obvious implementation detail leak.

Due to the last 3 points I would generally recommend sticking with manual registration in this scenario. Hope this helps, let me know if you have any other questions on this! 😄

This is great, thanks for the thorough answer!
For our purposes, I think manual registration is the right answer.

src/Compatibility/Core/src/iOS/Renderers/NavigationRenderer.cs

Sergio0694 · 2021-12-30T20:38:57Z

src/Controls/src/Core/RadioButtonGroupController.cs

-			WeakReferenceMessenger.Default.Register<RadioButtonGroupController, RadioButtonGroupNameChanged>(this, HandleRadioButtonGroupNameChanged);
-			WeakReferenceMessenger.Default.Register<RadioButtonGroupController, RadioButtonValueChanged>(this, HandleRadioButtonValueChanged);
+			WeakReferenceMessenger.Default.Register<RadioButtonGroupController, RadioButtonGroupSelectionChanged>(this, (r,m) => r.HandleRadioButtonGroupSelectionChanged(m));
+			WeakReferenceMessenger.Default.Register<RadioButtonGroupController, RadioButtonGroupNameChanged>(this, (r,m) => HandleRadioButtonGroupNameChanged(m));


Missed r. here? 🤔
I also would recommend making the lambdas static to make the code more explicit and less error prone 🙂

Suggested change

WeakReferenceMessenger.Default.Register<RadioButtonGroupController, RadioButtonGroupNameChanged>(this, (r,m) => HandleRadioButtonGroupNameChanged(m));

WeakReferenceMessenger.Default.Register<RadioButtonGroupController, RadioButtonGroupNameChanged>(this, static (r,m) => r.HandleRadioButtonGroupNameChanged(m));

I also would recommend making the lambdas static

Ah, added in C# 9. TIL.

drasticactions · 2021-12-30T22:20:22Z

Maybe this is a philosophical issue, I'm not sure I understand having a dependency on Windows Community Toolkit makes sense inside of MAUI itself. Is the goal here to integrate the Messenger codebase into MAUI and replace the existing stack wholesale?

hartez · 2021-12-30T22:56:57Z

Maybe this is a philosophical issue, I'm not sure I understand having a dependency on Windows Community Toolkit makes sense inside of MAUI itself. Is the goal here to integrate the Messenger codebase into MAUI and replace the existing stack wholesale?

Well, that's why I opened this up - so we could have the discussion.

This is in the same vein as moving MAUI to use Microsoft.Extensions.DependencyInjection rather than relying on the minimal DependencyService we had in Forms.

While it's nice that Forms shipped with MessagingCenter built-in, MC has problems. The API is awkward, the performance is sub-par, and it's likely got some memory leak issues. Also, the maintenance burden falls on the MAUI team, which is kind of a comparative advantage fail. Most folks would not recommend anyone use MC outside of a Forms/MAUI.Controls project, and even within a project using those SDKs, the recommendation would be to use something else if you're doing a lot of messaging.

So I think it's worth considering the option of obsoleting MessagingCenter and using something else with a better API and performance characteristics within MAUI.Controls. And freeing up maintenance energy for things which are more core to MAUI.

I don't have a particular problem with the dependency. (I don't love the MVVM part of the namespace, but whatever.) But if the dependency is a worry, maybe there's a case to be made for moving Messenger somewhere else?

Also, I should point out that this dependency is entirely within MAUI.Controls - this is not a dependency in MAUI.Core.

drasticactions · 2021-12-31T04:24:18Z

I think the difference between Microsoft.Extensions.DependencyInjection and the Windows Toolkit is that the DependencyInjection code is, I believe, a specific library that's only does that (Technically as part of a bunch of extensions), and are core component shared by several projects.

This toolkit has other features beyond just the messaging stuff, and that's also being brought into anyone who consumes MAUI Controls, whether they want it or not. It would be like automatically including parts of the MAUI Community Toolkit into MAUI itself because it has helper functions that would make MAUI development easier. IMO that's not ideal. But moving the stuff that would help into a shared place is the best of both worlds.

I do think changing it out is a good idea, and standardizing on one approach is 💯. Maybe if this specific part of the toolkit was refactored into its own library, then both could use it separately?

TheCodeTraveler · 2021-12-31T23:28:43Z

Another alternative is to promote CommunityToolkit.MVVM’s Messenger into .NET MAUI.

This is one of the primary goals of the Community Toolkits, to promote their features into the .NET Libraries.

Similarly, we recently promoted CommunityToolkit.Maui’s WeakEventManager into .NET MAUI.

Sergio0694 · 2022-01-01T02:12:18Z

"Windows Community Toolkit"

@drasticactions forgive me for this small nitpick, but the MVVM Toolkit is from the .NET Community Toolkit, not the Windows Community Toolkit. We split all the .NET libraries that were originally in the Windows Community Toolkit off into this new toolkit to make it clearer that it's completely decoupled from anything related to Windows 😊

"Another alternative is to promote CommunityToolkit.MVVM’s Messenger into .NET MAUI."

@brminnick The issue I have with this though is that this would mean that it would then be locked to MAUI, whereas one of the core principles of the MVVM Toolkit (or really, the .NET Community Toolkit as a whole) is that it's not dependent on any specific runtime or framework. There are many users of the MVVM Toolkit that eg. use WPF, UWP, Uno, even Unity, or more. We can't really remove the feature from there and only make it available through MAUI. This means that if we did want to promote this to be inbox into MAUI, we'd have to explore into eg. sharing the code there, under a different namespace. Still perfectl doable, just pointing out that simply moving the APIs to MAUI and removing it from the MVVM Toolkit isn't ideal 😅

TheCodeTraveler · 2022-01-01T03:11:13Z

simply moving the APIs to MAUI and removing it from the MVVM Toolkit isn't ideal

Totally. I'd recommend adding it to .NET MAUI and keeping it in CommunityToolkit.MVVM. If it is promoted into .NET, then we'd remove it from CommunityToolkit.MVVM.

drasticactions · 2022-01-01T18:21:02Z

"Windows Community Toolkit"

@drasticactions forgive me for this small nitpick, but the MVVM Toolkit is from the .NET Community Toolkit, not the Windows Community Toolkit. We split all the .NET libraries that were originally in the Windows Community Toolkit off into this new toolkit to make it clearer that it's completely decoupled from anything related to Windows 😊

It's not the name of it that matters here, it's that it's bringing in a community toolkit (that has a bunch of features) into the core Controls platform, to specifically handle one feature. IMO if we feel this messenger code is good enough for everyone's apps out of the box, we should either bake it directly into MAUI by promoting the code to live inside this repo, or move this messenger code into its own separate library and treat it like the DependencyInjection libraries.

GalaxiaGuy · 2022-01-02T16:56:29Z

A bit of an aside...

If dotnet community toolkit became a dependency, would it be worth seeing what else from it could be used? For example replacing Microsoft.Maui.Controls.Command with CommunityToolkit.Mvvm.Input.RelayCommand?

hartez · 2022-01-03T16:32:54Z

or move this messenger code into its own separate library and treat it like the DependencyInjection libraries.

What are the obstacles to doing something like this? Any dependencies or other issues that would prevent Messenger from becoming a standalone library usable by the Toolkit, WPF, UWP, MAUI, etc.?

Sergio0694 · 2022-01-03T16:37:23Z

"Any dependencies or other issues that would prevent Messenger from becoming a standalone library usable by the Toolkit, WPF, UWP, MAUI, etc.?"

No, the MVVM Toolkit (or really the .NET Community Toolkit as a whole) has no dependencies on any UI frameworks, it only multi-targets .NET Standard 2.0, 2.1, and .NET 6. It was one of the core reasons why we split stuff into the new Toolkit 🙂

NuGet.config

mattleibow · 2023-01-27T11:36:21Z

After a stack of discussion here and offline I think we reached a place where we can't depend on the community toolkit since part of them depend on us :)

I did have a spike a few different ways over here: #12910

I think with the way things are now, the page knows which window it is in and all windows have the alert manager. So in the end, the page can just do this.Window.AlertManager.DisplayAlert() and no fancy things.

That PR is a bit stalled as I need to focus on the upcoming net8 preview, but once I figure out the test failures it is a pretty minimal set of chnages.

TheCodeTraveler · 2023-01-27T11:52:12Z

where we can't depend on the community toolkit since part of them depend on us

Hey Matt! Can you elaborate on this?

The .NET MAUI Community Toolkit does have a dependency on .NET MAUI (obviously).

But the Messaging Center APIs live in the MVVM Community Toolkit which does not have a dependency on .NET MAUI.

And the .NET MAUI Community Toolkit does not have a dependency on the MVVM Community Toolkit.

tl;dr - I'm confused where a dependency conflict exists

mattleibow · 2023-01-27T11:59:37Z

Ah this is not directly a reference from mvvm to maui, but rather the concept of the community toolkits depending the core frameworks. Should have been more clear on this. More talking conceptually.

But either way, using a messaging system to talk from child to parent was a tiny bit overkill.

hartez added 3 commits December 28, 2021 15:58

testing moving the Android page stuff

de967b9

Update the Page stuff for all platforms

39867d8

Replace the rest of the MC stuff

6cb67bb

Sergio0694 suggested changes Dec 29, 2021

View reviewed changes

Convert message classes to records

4a01020

Sergio0694 reviewed Dec 30, 2021

View reviewed changes

src/Compatibility/Core/src/iOS/Renderers/NavigationRenderer.cs Outdated Show resolved Hide resolved

hartez added 2 commits December 30, 2021 12:52

Don't make empty classes into records

c4e8f49

Fix some closures and method group usage

63a4020

Sergio0694 reviewed Dec 30, 2021

View reviewed changes

hartez added 2 commits December 30, 2021 13:50

Mark lambdas static

b425af7

Clean up more closures and method groups

ca9f0d7

GalaxiaGuy mentioned this pull request Jan 2, 2022

Weak PropertyChanged on ObservableObject CommunityToolkit/dotnet#80

Open

jsuarezruiz added the legacy-area-perf Startup / Runtime performance label Jan 10, 2022

Redth reviewed Jan 24, 2022

View reviewed changes

NuGet.config Outdated Show resolved Hide resolved

Update NuGet.config

b5125da

rmarinho force-pushed the main branch from 00bb62c to 41406f2 Compare March 14, 2022 12:49

jfversluis mentioned this pull request Jul 15, 2022

Add Obsolete attribute to MessagingCenter #8765

Merged

jfversluis mentioned this pull request Oct 14, 2022

MessagingCenter is not thread safe #10670

Closed

mattleibow closed this Jan 27, 2023

mattleibow deleted the messenger branch January 27, 2023 11:36

github-actions bot locked and limited conversation to collaborators Dec 21, 2023

Eilon added the perf/general The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) label May 10, 2024

	WeakReferenceMessenger.Default.Send(new CloseContextActionsMessage());
	WeakReferenceMessenger.Default.Send<CloseContextActionsMessage>();

	WeakReferenceMessenger.Default.Register<CloseContextActionsMessage>(this, (recipient, msg) => CloseContextActions());
	WeakReferenceMessenger.Default.Register<ListViewAdapter, CloseContextActionsMessage>(this, (r, _) => r.CloseContextActions());

Conversation

hartez commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sergio0694 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hartez Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sergio0694 Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drasticactions commented Dec 30, 2021

Uh oh!

hartez commented Dec 30, 2021

Uh oh!

drasticactions commented Dec 31, 2021

Uh oh!

TheCodeTraveler commented Dec 31, 2021

Uh oh!

Sergio0694 commented Jan 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheCodeTraveler commented Jan 1, 2022

Uh oh!

drasticactions commented Jan 1, 2022

Uh oh!

GalaxiaGuy commented Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hartez commented Jan 3, 2022

Uh oh!

Sergio0694 commented Jan 3, 2022

Uh oh!

Uh oh!

mattleibow commented Jan 27, 2023

Uh oh!

TheCodeTraveler commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattleibow commented Jan 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

hartez commented Dec 29, 2021 •

edited

Loading

hartez Dec 30, 2021 •

edited

Loading

Sergio0694 Dec 30, 2021 •

edited

Loading

Sergio0694 commented Jan 1, 2022 •

edited

Loading

GalaxiaGuy commented Jan 2, 2022 •

edited

Loading

TheCodeTraveler commented Jan 27, 2023 •

edited

Loading