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.

Sign up for GitHub

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

Jump to bottom

[core] use Lazy<T> for MauiContext properties #8033

Merged
merged 1 commit into from
Jun 16, 2022
Merged

[core] use Lazy<T> for MauiContext properties #8033

merged 1 commit into from
Jun 16, 2022

Conversation

jonathanpeppers
Copy link
Member

Context: https://github.com/unoplatform/performance/tree/master/src/dopes/DopeTestMaui

Building upon #7996 and #8001, I noticed while profiling the sample
app putting N Label on the screen:

405.02ms (2.3%) microsoft.maui!Microsoft.Maui.MauiContext.get_Context()
77.90ms (0.44%) microsoft.maui!Microsoft.Maui.MauiContext.get_Handlers()

Meaning that around ~2.74% of the time was spent just calling these two
properties. These properties hit Microsoft.Extensions each time to
locate the service:

public Android.Content.Context? Context => _services.GetService<Android.Content.Context>();

These are core-services that wouldn't change after
startup, so should we just use Lazy<T> instead of doing a call like
this each time?

I also made use of ANDROID, as that is the preferred define to use
in .NET 6+ instead of __ANDROID__:

https://github.com/dotnet/designs/blob/a447d77bb53d189746ccd80c2d814064c2b6c606/accepted/2020/net5/net5.md#vary-implementation

Results

A Release build on a Pixel 5 device, I was getting:

Before: 81.70 Dopes/s
After:  91.94 Dopes/s

image

@jonathanpeppers
[core] use Lazy<T> for MauiContext properties
5c4a499 
Context: https://github.com/unoplatform/performance/tree/master/src/dopes/DopeTestMaui

Building upon dotnet#7996 and dotnet#8001, I noticed while profiling the sample
app putting N `Label` on the screen:

    405.02ms (2.3%) microsoft.maui!Microsoft.Maui.MauiContext.get_Context()
    77.90ms (0.44%) microsoft.maui!Microsoft.Maui.MauiContext.get_Handlers()

Meaning that around ~2.74% of the time was spent just calling these two
properties. These properties hit Microsoft.Extensions each time to
locate the service:

    public Android.Content.Context? Context => _services.GetService<Android.Content.Context>();

These are core-services that wouldn't change after
startup, so should we just use `Lazy<T>` instead of doing a call like
this each time?

I also made use of `ANDROID`, as that is the preferred define to use
in .NET 6+ instead of `__ANDROID__`:

https://github.com/dotnet/designs/blob/a447d77bb53d189746ccd80c2d814064c2b6c606/accepted/2020/net5/net5.md#vary-implementation

~~ Results ~~

A `Release` build on a Pixel 5 device, I was getting:

    Before: 81.70 Dopes/s
    After:  91.94 Dopes/s
@jonathanpeppers jonathanpeppers added the legacy-area-perf Startup / Runtime performance label Jun 15, 2022
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request Jun 15, 2022
@jonathanpeppers
[android] reduce JNI calls during layout measurement
9163310 
Context: https://github.com/unoplatform/performance/tree/master/src/dopes/DopeTestMaui

Building upon dotnet#7996, dotnet#8001, and dotnet#8033, I noticed while profiling the
sample app putting N Label on the screen:

    783.92ms (6.2%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.GetDesiredSizeFromHandler(Microsoft.Maui.IViewHandler,double,double)

So around %6 of the time spend just measuring.

Looking through the call stack, I can see 3 JNI calls happening:

    932.51ms (7.4%)  mono.android!Android.Views.View.Measure(int,int)
    115.53ms (0.91%) mono.android!Android.Views.View.get_MeasuredWidth()
     96.97ms (0.77%) mono.android!Android.Views.View.get_MeasuredHeight()

So, we could write a Java method that calls all three of these and
somehow returns the `MeasuredWidth` and `MeasuredHeight`. After a
little research, it seemed the best approach here was to "pack" two
integers into a `long`. If we tried to return some Java object
instead, then we'd have the same number of JNI calls to get the
integers out.

I found a couple links describing how to "pack" an `int` into a `long`:

* Java: https://stackoverflow.com/a/12772968
* C#: https://stackoverflow.com/a/827267

Which after some testing, arrives at:

    public static long measureAndGetWidthAndHeight(View view, int widthMeasureSpec, int heightMeasureSpec) {
        view.measure(widthMeasureSpec, heightMeasureSpec);
        int width = view.getMeasuredWidth();
        int height = view.getMeasuredHeight();
        return ((long)width << 32) | (height & 0xffffffffL);
    }

Unpacked in C# such as:

    var packed = PlatformInterop.MeasureAndGetWidthAndHeight(platformView, widthSpec, heightSpec);
    var measuredWidth = (int)(packed >> 32);
    var measuredHeight = (int)(packed & 0xffffffffL);

Reducing 3 JNI calls in every `View`'s layout to 1.

~~ Results ~~

A `Release` build on a Pixel 5 device, I was getting:

    Before:  91.94 Dopes/s
    After:  102.45 Dopes/s

After profiling again, it drops the % time spent in
`GetDesiredSizeFromHandler`:

    528.96ms (4.5%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.GetDesiredSizeFromHandler

So the added math is negligible compared to reduced JNI calls.
@jonathanpeppers jonathanpeppers mentioned this pull request Jun 15, 2022
Merged
@PureWeen
Copy link
Member

PureWeen commented Jun 15, 2022 •
edited
Loading

@jonathanpeppers

These properties hit Microsoft.Extensions each time to locate the service:

Is this slower because it's going through our WrappedServiceProvider ? Should we rework how the lookup works inside WrappedServiceProvider ?

It looks like from your measurements the get_Context call is the slower of the two and I'm pretty sure that's the one that'll hit the WrappedServiceProvider and won't make it to Extensions at all.

PureWeen pushed a commit that referenced this pull request Jun 15, 2022
@jonathanpeppers
[android] reduce JNI calls during layout measurement (#8034)
06e5c08 
Context: https://github.com/unoplatform/performance/tree/master/src/dopes/DopeTestMaui

Building upon #7996, #8001, and #8033, I noticed while profiling the
sample app putting N Label on the screen:

    783.92ms (6.2%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.GetDesiredSizeFromHandler(Microsoft.Maui.IViewHandler,double,double)

So around %6 of the time spend just measuring.

Looking through the call stack, I can see 3 JNI calls happening:

    932.51ms (7.4%)  mono.android!Android.Views.View.Measure(int,int)
    115.53ms (0.91%) mono.android!Android.Views.View.get_MeasuredWidth()
     96.97ms (0.77%) mono.android!Android.Views.View.get_MeasuredHeight()

So, we could write a Java method that calls all three of these and
somehow returns the `MeasuredWidth` and `MeasuredHeight`. After a
little research, it seemed the best approach here was to "pack" two
integers into a `long`. If we tried to return some Java object
instead, then we'd have the same number of JNI calls to get the
integers out.

I found a couple links describing how to "pack" an `int` into a `long`:

* Java: https://stackoverflow.com/a/12772968
* C#: https://stackoverflow.com/a/827267

Which after some testing, arrives at:

    public static long measureAndGetWidthAndHeight(View view, int widthMeasureSpec, int heightMeasureSpec) {
        view.measure(widthMeasureSpec, heightMeasureSpec);
        int width = view.getMeasuredWidth();
        int height = view.getMeasuredHeight();
        return ((long)width << 32) | (height & 0xffffffffL);
    }

Unpacked in C# such as:

    var packed = PlatformInterop.MeasureAndGetWidthAndHeight(platformView, widthSpec, heightSpec);
    var measuredWidth = (int)(packed >> 32);
    var measuredHeight = (int)(packed & 0xffffffffL);

Reducing 3 JNI calls in every `View`'s layout to 1.

~~ Results ~~

A `Release` build on a Pixel 5 device, I was getting:

    Before:  91.94 Dopes/s
    After:  102.45 Dopes/s

After profiling again, it drops the % time spent in
`GetDesiredSizeFromHandler`:

    528.96ms (4.5%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.GetDesiredSizeFromHandler

So the added math is negligible compared to reduced JNI calls.
@jonathanpeppers
Copy link
Member Author

Yeah it is WrappedServiceProvider:

image

maui-app_20220614_112919.speedscope.zip

But it should just be hitting that ConcurrentDictionary. This might only be a problem because .Context is called so many times.

@PureWeen
Copy link
Member

@jonathanpeppers Yea, these changes are just a little interesting because AFAICR this is originally how we were doing it but then we generalized it into the ConcurrentDictionary

The amount of things we add into the WrappedServiceProvider is contained and it's not something that users can add to, so I kind of wonder if we need to just get rid of the ConcurrentDictionary and just go back to having a single property for everything.

The code now how it runs just feels a bit funny

  • We add a specific scoped service like Android.Context that goes into the ConcurrentDict
  • Then the first time it's retrieved we store it locally so it's no longer retrieved from the ConcurrentDict

Meanwhile all user requests are now going through the ConcurrentDict before hitting the Ms.Ext's

Not saying we need to block this PR on any of this but it feels like we should just get rid of the ConcurrentDict and just deal with manually adding these Scoped properties that we create.

@mattleibow ^

@PureWeen PureWeen mentioned this pull request Jun 16, 2022
Closed
@PureWeen
Copy link
Member

#8128

@PureWeen PureWeen merged commit b357756 into dotnet:main Jun 16, 2022
@jonathanpeppers jonathanpeppers deleted the MauiContextServices branch June 16, 2022 22:13
@jonathanpeppers jonathanpeppers mentioned this pull request Jun 21, 2022
Closed
rmarinho pushed a commit that referenced this pull request Jun 23, 2022
@jonathanpeppers @rmarinho
[android] reduce JNI calls during layout measurement (#8034)
f4e559a 
Context: https://github.com/unoplatform/performance/tree/master/src/dopes/DopeTestMaui

Building upon #7996, #8001, and #8033, I noticed while profiling the
sample app putting N Label on the screen:

    783.92ms (6.2%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.GetDesiredSizeFromHandler(Microsoft.Maui.IViewHandler,double,double)

So around %6 of the time spend just measuring.

Looking through the call stack, I can see 3 JNI calls happening:

    932.51ms (7.4%)  mono.android!Android.Views.View.Measure(int,int)
    115.53ms (0.91%) mono.android!Android.Views.View.get_MeasuredWidth()
     96.97ms (0.77%) mono.android!Android.Views.View.get_MeasuredHeight()

So, we could write a Java method that calls all three of these and
somehow returns the `MeasuredWidth` and `MeasuredHeight`. After a
little research, it seemed the best approach here was to "pack" two
integers into a `long`. If we tried to return some Java object
instead, then we'd have the same number of JNI calls to get the
integers out.

I found a couple links describing how to "pack" an `int` into a `long`:

* Java: https://stackoverflow.com/a/12772968
* C#: https://stackoverflow.com/a/827267

Which after some testing, arrives at:

    public static long measureAndGetWidthAndHeight(View view, int widthMeasureSpec, int heightMeasureSpec) {
        view.measure(widthMeasureSpec, heightMeasureSpec);
        int width = view.getMeasuredWidth();
        int height = view.getMeasuredHeight();
        return ((long)width << 32) | (height & 0xffffffffL);
    }

Unpacked in C# such as:

    var packed = PlatformInterop.MeasureAndGetWidthAndHeight(platformView, widthSpec, heightSpec);
    var measuredWidth = (int)(packed >> 32);
    var measuredHeight = (int)(packed & 0xffffffffL);

Reducing 3 JNI calls in every `View`'s layout to 1.

~~ Results ~~

A `Release` build on a Pixel 5 device, I was getting:

    Before:  91.94 Dopes/s
    After:  102.45 Dopes/s

After profiling again, it drops the % time spent in
`GetDesiredSizeFromHandler`:

    528.96ms (4.5%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.GetDesiredSizeFromHandler

So the added math is negligible compared to reduced JNI calls.
rmarinho pushed a commit that referenced this pull request Jun 23, 2022
@jonathanpeppers @rmarinho
[core] use Lazy<T> for MauiContext properties (#8033)
6e7d713 
Context: https://github.com/unoplatform/performance/tree/master/src/dopes/DopeTestMaui

Building upon #7996 and #8001, I noticed while profiling the sample
app putting N `Label` on the screen:

    405.02ms (2.3%) microsoft.maui!Microsoft.Maui.MauiContext.get_Context()
    77.90ms (0.44%) microsoft.maui!Microsoft.Maui.MauiContext.get_Handlers()

Meaning that around ~2.74% of the time was spent just calling these two
properties. These properties hit Microsoft.Extensions each time to
locate the service:

    public Android.Content.Context? Context => _services.GetService<Android.Content.Context>();

These are core-services that wouldn't change after
startup, so should we just use `Lazy<T>` instead of doing a call like
this each time?

I also made use of `ANDROID`, as that is the preferred define to use
in .NET 6+ instead of `__ANDROID__`:

https://github.com/dotnet/designs/blob/a447d77bb53d189746ccd80c2d814064c2b6c606/accepted/2020/net5/net5.md#vary-implementation

~~ Results ~~

A `Release` build on a Pixel 5 device, I was getting:

    Before: 81.70 Dopes/s
    After:  91.94 Dopes/s
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2023
@Eilon Eilon added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jsuarezruiz jsuarezruiz jsuarezruiz approved these changes

@mattleibow mattleibow Awaiting requested review from mattleibow

@PureWeen PureWeen Awaiting requested review from PureWeen

Assignees
No one assigned
Labels
fixed-in-7.0.0-rc.1.6683 legacy-area-perf Startup / Runtime performance t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
.NET 7 MAUI Performance
Status: Done & Blogged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jonathanpeppers @PureWeen @jsuarezruiz @Eilon @samhouts