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

[controls] improve perf of "merged" ResourceDictionary lookups #21334

Merged

Conversation

jonathanpeppers
Copy link
Member

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:

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 just 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.

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 added the legacy-area-perf Startup / Runtime performance label Mar 20, 2024
@jonathanpeppers jonathanpeppers marked this pull request as ready for review March 20, 2024 18:24
@jonathanpeppers jonathanpeppers requested a review from a team as a code owner March 20, 2024 18:24
@StephaneDelcroix StephaneDelcroix merged commit 8ca7648 into dotnet:main Mar 27, 2024
47 checks passed
@jonathanpeppers jonathanpeppers deleted the MergedResourceDictionaries branch March 27, 2024 16:33
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
@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.
Labels
fixed-in-8.0.20 fixed-in-9.0.0-preview.3.10457 legacy-area-perf Startup / Runtime performance t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants