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

[android] avoid OnLayout() for Label #21291

Merged
merged 1 commit into from Mar 27, 2024
Merged

[android] avoid OnLayout() for Label #21291

merged 1 commit into from Mar 27, 2024

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Mar 18, 2024 •
edited

Context: #18505
Context: https://github.com/dotnet/maui/files/13251041/MauiCollectionView.zip
Context: #21229 (review)

In profiling scrolling of an app with a <CollectionView/> and 12
<Label/>s, we see time spent in:

1.9% Microsoft.Maui!Microsoft.Maui.Platform.MauiTextView.OnLayout(bool,int,int,int,int)

This is a callback from Java to C#, which has a performance cost.
Reviewing the code, we would only need to make this callback at all
if Label.FormattedText is not null. The bulk of all Label's can
avoid this call?

To do this:

  • Write a new PlatformAppCompatTextView.java that override onLayout()

  • It only calls onLayoutFormatted() if a isFormatted boolean
    field is true

  • We can set isFormatted if a formatted string is passed in, such as:
    isFormatted = !(text instanceof String)

With this change in place, the above MauiTextView.OnLayout() call is
completely gone from dotnet-trace output. Scrolling the sample also
"feels" a bit snappier.

This should improve the performance of all non-formatted Labels on
Android.

This is the mininum amount of API changes possible -- which seems like
what we should go for if we ship this change in .NET 8 servicing.

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request Mar 20, 2024
@jonathanpeppers
[controls] improve perf of "merged" ResourceDictionary lookups
c6a793e 
Applies to: dotnet#18505
Context: https://github.com/dotnet/maui/files/13251041/MauiCollectionView.zip

I profiled the above sample with `dotnet-trace` with the following PRs
applied locally:

* dotnet#21229
* dotnet#21291

While scrolling, a lot of time is spent in `ResourceDictionary`
lookups on an Android Pixel 5 device:

    2.0% Microsoft.Maui.Controls!Microsoft.Maui.Controls.ResourceDictionary.TryGetValue(string,object&)

Drilling in, I can see System.Linq's `Reverse()` method:

    0.56% System.Linq!System.Linq.Enumerable.ReverseIterator<TSource_REF>.MoveNext()
    0.14% System.Linq!System.Linq.Enumerable.Reverse(System.Collections.Generic.IEnumerable`1<TSource_REF>)
    0.04% System.Linq!System.Linq.Enumerable.ReverseIterator<TSource_REF>..ctor(System.Collections.Generic.IEnumerable`1<TSource_REF>)
    0.04% System.Linq!System.Linq.Enumerable.ReverseIterator<TSource_REF>.Dispose()

`Reverse()` can be problematic as it can sometimes create a copy of
the entire collection, in order to sort in reverse. We can juse use a
reverse `for`-loop instead.

The indexer, we can also avoid a double-lookup:

    if (dict.ContainsKey(index))
        return dict[index];

And instead do:

    if (dict.TryGetValue(index, out var value))
        return value;

The MAUI project template seems to setup a few "merged"
`ResourceDictionary` as it contains `Styles.xaml`, so this is why this
code path is being hit.

I wrote a BenchmarkDotNet benchmark, and it indicates the collection
is being copied, as the 872 bytes of allocation occur:

| Method      | key         | Mean      | Error    | StdDev   | Gen0   | Allocated |
|------------ |------------ |----------:|---------:|---------:|-------:|----------:|
| TryGetValue | key0        |  11.45 ns | 0.026 ns | 0.023 ns |      - |         - |
| Indexer     | key0        |  24.72 ns | 0.133 ns | 0.118 ns |      - |         - |
| TryGetValue | merged99,99 | 117.06 ns | 2.334 ns | 2.497 ns | 0.1042 |     872 B |
| Indexer     | merged99,99 | 145.60 ns | 2.737 ns | 2.286 ns | 0.1042 |     872 B |

With these changes in place, I see less time spent inside:

    0.91% Microsoft.Maui.Controls!Microsoft.Maui.Controls.ResourceDictionary.TryGetValue(string,object&)

The benchmark no longer allocates either:

| Method      | key         | Mean      | Error     | StdDev    | Allocated |
|------------ |------------ |----------:|----------:|----------:|----------:|
| TryGetValue | key0        |  11.92 ns |  0.094 ns |  0.084 ns |         - |
| Indexer     | merged99,99 |  23.12 ns |  0.418 ns |  0.391 ns |         - |
| Indexer     | key0        |  24.20 ns |  0.485 ns |  0.453 ns |         - |
| TryGetValue | merged99,99 |  29.09 ns |  0.296 ns |  0.262 ns |         - |

This should improve the performance "parenting" of any MAUI view on
all platforms -- as well as scrolling `CollectionView`.
@jonathanpeppers jonathanpeppers mentioned this pull request Mar 20, 2024
Merged
@jonathanpeppers
[android] avoid OnLayout() for Label
c8aebdc 
Context: dotnet#18505
Context: https://github.com/dotnet/maui/files/13251041/MauiCollectionView.zip
Context: dotnet#21229 (review)

In profiling scrolling of an app with a `<CollectionView/>` and 12
`<Label/>`s, we see time spent in:

    1.9% Microsoft.Maui!Microsoft.Maui.Platform.MauiTextView.OnLayout(bool,int,int,int,int)

This is a callback from Java to C#, which has a performance cost.
Reviewing the code, we would only need to make this callback *at all*
if `Label.FormattedText` is not `null`. The bulk of all `Label`'s can
avoid this call?

To do this:

* Write a new `PlatformAppCompatTextView.java` that override `onLayout()`

* It only calls `onLayoutFormatted()` if a `isFormatted` `boolean`
  field is `true`

* We can set `isFormatted` if a formatted string is such as:
  `isFormatted = !(text instanceof String)`

With this change in place, the above `MauiTextView.OnLayout()` call is
completely gone from `dotnet-trace` output. Scrolling the sample also
"feels" a bit snappier.

This should improve the performance of all non-formatted `Label`s on
Android.

This is the mininum amount of API changes possible -- which seems like
what we should go for if we ship this change in .NET 8 servicing.
jonathanpeppers
jonathanpeppers commented Mar 26, 2024
View reviewed changes
src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformAppCompatTextView.java
Comment on lines +20 to +24
@Override
public void setText(CharSequence text, BufferType type) {
isFormatted = !(text instanceof String);
super.setText(text, type);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that calling TextView.Text = "foo" will call inside here:

The C# TextView.Text property is bound to setText(CharSequence).

@jonathanpeppers jonathanpeppers marked this pull request as ready for review March 26, 2024 13:49
@jonathanpeppers jonathanpeppers requested a review from a team as a code owner March 26, 2024 13:49
@jonathanpeppers jonathanpeppers merged commit 83960b0 into dotnet:main Mar 27, 2024
47 checks passed
@jonathanpeppers jonathanpeppers deleted the PlatformAppCompatTextView branch March 27, 2024 13:15
StephaneDelcroix pushed a commit that referenced this pull request Mar 27, 2024
@jonathanpeppers
[controls] improve perf of "merged" ResourceDictionary lookups (#21334)
8ca7648 
Applies to: #18505
Context: https://github.com/dotnet/maui/files/13251041/MauiCollectionView.zip

I profiled the above sample with `dotnet-trace` with the following PRs
applied locally:

* #21229
* #21291

While scrolling, a lot of time is spent in `ResourceDictionary`
lookups on an Android Pixel 5 device:

    2.0% Microsoft.Maui.Controls!Microsoft.Maui.Controls.ResourceDictionary.TryGetValue(string,object&)

Drilling in, I can see System.Linq's `Reverse()` method:

    0.56% System.Linq!System.Linq.Enumerable.ReverseIterator<TSource_REF>.MoveNext()
    0.14% System.Linq!System.Linq.Enumerable.Reverse(System.Collections.Generic.IEnumerable`1<TSource_REF>)
    0.04% System.Linq!System.Linq.Enumerable.ReverseIterator<TSource_REF>..ctor(System.Collections.Generic.IEnumerable`1<TSource_REF>)
    0.04% System.Linq!System.Linq.Enumerable.ReverseIterator<TSource_REF>.Dispose()

`Reverse()` can be problematic as it can sometimes create a copy of
the entire collection, in order to sort in reverse. We can juse use a
reverse `for`-loop instead.

The indexer, we can also avoid a double-lookup:

    if (dict.ContainsKey(index))
        return dict[index];

And instead do:

    if (dict.TryGetValue(index, out var value))
        return value;

The MAUI project template seems to setup a few "merged"
`ResourceDictionary` as it contains `Styles.xaml`, so this is why this
code path is being hit.

I wrote a BenchmarkDotNet benchmark, and it indicates the collection
is being copied, as the 872 bytes of allocation occur:

| Method      | key         | Mean      | Error    | StdDev   | Gen0   | Allocated |
|------------ |------------ |----------:|---------:|---------:|-------:|----------:|
| TryGetValue | key0        |  11.45 ns | 0.026 ns | 0.023 ns |      - |         - |
| Indexer     | key0        |  24.72 ns | 0.133 ns | 0.118 ns |      - |         - |
| TryGetValue | merged99,99 | 117.06 ns | 2.334 ns | 2.497 ns | 0.1042 |     872 B |
| Indexer     | merged99,99 | 145.60 ns | 2.737 ns | 2.286 ns | 0.1042 |     872 B |

With these changes in place, I see less time spent inside:

    0.91% Microsoft.Maui.Controls!Microsoft.Maui.Controls.ResourceDictionary.TryGetValue(string,object&)

The benchmark no longer allocates either:

| Method      | key         | Mean      | Error     | StdDev    | Allocated |
|------------ |------------ |----------:|----------:|----------:|----------:|
| TryGetValue | key0        |  11.92 ns |  0.094 ns |  0.084 ns |         - |
| Indexer     | merged99,99 |  23.12 ns |  0.418 ns |  0.391 ns |         - |
| Indexer     | key0        |  24.20 ns |  0.485 ns |  0.453 ns |         - |
| TryGetValue | merged99,99 |  29.09 ns |  0.296 ns |  0.262 ns |         - |

This should improve the performance "parenting" of any MAUI view on
all platforms -- as well as scrolling `CollectionView`.
@jonmdev jonmdev mentioned this pull request Apr 4, 2024
Open
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) area-controls-label Label, Span 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.
Reviewers

@jsuarezruiz jsuarezruiz jsuarezruiz approved these changes

@jfversluis jfversluis Awaiting requested review from jfversluis jfversluis is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

@rmarinho rmarinho Awaiting requested review from rmarinho rmarinho is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

Assignees
No one assigned
Labels
area-controls-label Label, Span platform/android 🤖 t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
MAUI SDK Ongoing
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jonathanpeppers @jsuarezruiz @Eilon