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

OnPlatform could be simplified at build time #4716

Closed
StephaneDelcroix opened this issue Feb 16, 2022 · 0 comments · Fixed by #4829
Closed

OnPlatform could be simplified at build time #4716

StephaneDelcroix opened this issue Feb 16, 2022 · 0 comments · Fixed by #4829
Assignees
Labels
area-xaml XAML, CSS, Triggers, Behaviors fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1! proposal/open t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)

Comments

@StephaneDelcroix
Copy link
Contributor

StephaneDelcroix commented Feb 16, 2022

Description

For XamlC scenarios (Release builds), as we now multi-targeting, {OnPlatform} markup extensions could be simplified at build time.

this Xaml code

<Grid Padding="{OnPlatform iOS='30,60,30,30', Default='30'}" />

should be evaluated as

<Grid Padding="30,60,30,30" />

on iOS, and Padding="30" on other platforms.

This is possible as the platform won't change while the app is build.

To avoid writing too much specialised code around this, I'd recommend adding this rule as a visitor early in the parsing, before the IL generation, so the IL generator (XamlC) won't see any difference.

/cc @eerhardt

Public API Changes

/

Intended Use-Case

/

@StephaneDelcroix StephaneDelcroix self-assigned this Feb 16, 2022
@jsuarezruiz jsuarezruiz added the area-xaml XAML, CSS, Triggers, Behaviors label Feb 16, 2022
@jsuarezruiz jsuarezruiz added this to Ready for implementation in Enhancements Feb 16, 2022
@StephaneDelcroix StephaneDelcroix added the legacy-area-perf Startup / Runtime performance label Feb 16, 2022
StephaneDelcroix added a commit that referenced this issue Feb 22, 2022
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
StephaneDelcroix added a commit that referenced this issue Mar 10, 2022
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
@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2022
@samhouts samhouts added the fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1! label Feb 17, 2023
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-xaml XAML, CSS, Triggers, Behaviors fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1! proposal/open t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
Enhancements
Ready for implementation
Development

Successfully merging a pull request may close this issue.

4 participants