[X] Simplify OnPlatformExtension #4829
Conversation
When the platform is known at compile time, and it's known by default as we are multitargetting, replace OnPlatfomExtension by the actual value. This is done early in the parsing so every other optimization is unaffected. From a perf point of view, there are multiple gains: - OnPlatformExtension is executed at runtime, and makes use of reflection. Removing OnPlatformExtensions from the tree reduces this to nothing. - Values in OnPlatformExtension never benefitted from compiledtypeconverters - not generating the IL for the OnPlatform is huge win too. The unitest goes from 189 instructions to 42. fixes #4716
|
/cc @eerhardt |
| target = nameof(OnPlatformExtension.iOS); | ||
| if (TargetFramework.Contains("-macos")) | ||
| target = nameof(OnPlatformExtension.macOS); | ||
| if (TargetFramework.Contains("-maccatalyst")) |
There was a problem hiding this comment.
Is there no "Windows" platform extensions? Or do we not care in that case?
There was a problem hiding this comment.
This is kind of interesting since -windows could be wpf, or winforms or winappsdk in theory right? We only support winappsdk now, but who knows in the future... So perhaps target framework is not enough to check for -windows at some point...
There was a problem hiding this comment.
there's no 1-1 mapping between .net TFM for windows and Maui targeted platform
There was a problem hiding this comment.
What are the "platform extensions" for windows? How does that switch happen at runtime?
There was a problem hiding this comment.
https://github.com/dotnet/maui/blob/main/src/Controls/src/Xaml/MarkupExtensions/OnPlatformExtension.cs#L13-L21
the platform for the running app is set by the IPlatformApplication scaffolding the whole Maui thing
There was a problem hiding this comment.
#4491 added WinUI as a platform in the OnPlatform markup extension for windows...
So, today we only have 1 windows target, perhaps we should optimize for the now. If we need to, in the future could we optimize such that at least -windows only has a limited number of options to check through and perhaps on -windows it's a slower path for this xamlc, but that's probably fine anyway because it's not android.
There was a problem hiding this comment.
this might be a good time to review our current platform list, and names, and what defines a platform. should iPadOS be it's own platform, what about deprecating windows in favour of winui, wpf, ...
There was a problem hiding this comment.
Am I reading this correctly, in that if it's not windows, then there's just no optimization in the code path like there is for the other known tfm's? If that's the case I think this is fine as is and we should try and merge it in.
There was a problem hiding this comment.
yep, that's it, if the TFM is unlisted, we do nothing
| return; | ||
|
|
||
| string target = null; | ||
| if (TargetFramework.Contains("-android")) |
There was a problem hiding this comment.
TargetFramework doesn't change for the lifetime of this object. Does it make sense to do this switch every time?
There was a problem hiding this comment.
despite being in the Xaml project, this code is only ever executed by the compiler, and we're usually ok to sacrifice a few ms over code simplicity and readability. I'm 100% sure there's an alternative code that is as readable, as debuggable and as simple, but I'm not sure it will ever save the time we currently spend discussing it :)
|
there's also a discussion gaining some traction about doing conditionals in XAML that would make OnPlatform obsolete... I'll make a proposal soon |
When the platform is known at compile time, and it's known by default as
we are multitargetting, replace OnPlatfomExtension by the actual value.
This is done early in the parsing so every other optimization is
unaffected.
From a perf point of view, there are multiple gains:
reflection. Removing OnPlatformExtensions from the tree reduces this
to nothing.
compiledtypeconverters
goes from 189 instructions to 42.
fixes #4716
Description of Change
Implements #
Additions made
PR Checklist
Does this PR touch anything that might affect accessibility?
If any of the above checkboxes apply to your PR, then the PR will need to provide testing to demonstrate that accessibility still works.