[Mono.Android] improve performance of new Java arrays #6870
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonathanpeppers
commented
Mar 28, 2022
jonathanpeppers
changed the title
jonpryor
reviewed
Mar 29, 2022
Context: https://github.com/dotnet/maui/blob/79f2fef9397bbfe2f6aa1527e40f933712a5d2ad/src/Core/src/Platform/Android/ColorStateListExtensions.cs#L50-L51 When running a `dotnet new maui` app, we noticed the log: W monodroid: typemap: failed to map managed type to Java type: System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e (Module ID: 8e4cd939-3275-41c4-968d-d5a4376b35f5; Type token: 33554653) W monodroid-assembly: typemap: called from W monodroid-assembly: at Android.Runtime.JNIEnv.TypemapManagedToJava(Type ) W monodroid-assembly: at Android.Runtime.JNIEnv.GetJniName(Type ) W monodroid-assembly: at Android.Runtime.JNIEnv.FindClass(Type ) W monodroid-assembly: at Android.Runtime.JNIEnv.NewArray(Array , Type ) W monodroid-assembly: at Android.Runtime.JNIEnv.NewArray[Int32[]](Int32[][] ) W monodroid-assembly: at Android.Content.Res.ColorStateList..ctor(Int32[][] , Int32[] ) W monodroid-assembly: at Microsoft.Maui.Platform.ColorStateListExtensions.CreateButton(Int32 enabled, Int32 disabled, Int32 off, Int32 pressed) We see this ~17 times on startup. In this case, the Android binding is: public unsafe ColorStateList (int[][]? states, int[]? colors) : base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer) { //... IntPtr native_states = JNIEnv.NewArray (states); Looking at the stack trace, it seems like this is happening for every item in the array? We could probably call `FindClass (typeof (T))` once, and use it for every item in the array. This would avoid many typemap lookups. Previously this showed up in `dotnet trace` output for a Pixel 5: 6.05ms Mono.Android!Android.Runtime.JNIEnv.NewArray 4.84ms Mono.Android!Android.Runtime.JNIEnv.NewArray(System.Array,System.Type) After these changes: 5.04ms Mono.Android!Android.Runtime.JNIEnv.NewArray 3.83ms Mono.Android!Android.Runtime.JNIEnv.NewArray(System.Array,System.Type,intptr) This change seems to save ~1ms on startup for `dotnet new maui` on a Pixel 5 device.
jonathanpeppers
force-pushed
the
MultidimensionalNewArray
branch
from
e767445
to
5075992
Compare
jonathanpeppers
changed the title
jonpryor
reviewed
Mar 29, 2022
| @@ -1596,14 +1584,41 @@ public static IntPtr NewArray<T> (T[]? array) | |||
| return IntPtr.Zero; | |||
|
|
|||
| if (typeof (T).IsArray) { | |||
| return NewArray (array, typeof (T)); | |||
| IntPtr grefArrayClass = FindClass (typeof (T)); | |||
There was a problem hiding this comment.
Naming: this isn't the "class" of the array; this is the "class" of the element. grefElementClass would be a better name.
There was a problem hiding this comment.
Alas, grefArrayClass is consistent with existing usage in the other NewArray() method, but I was mentally confused the first time I read this… :-(
There was a problem hiding this comment.
I used grefArrayElementClass, as I found it in another place in this file.
jonathanpeppers
added a commit
to jonathanpeppers/maui
that referenced
this pull request
Mar 29, 2022
`dotnet trace` output of `dotnet new maui` on a Pixel 5 device shows:
7.54ms Microsoft.Maui!Microsoft.Maui.Platform.ColorStateListExtensions.CreateDefault
7.54ms Mono.Android!Android.Content.Res.ColorStateList..ctor(int[][],int[])
5.04ms Mono.Android!Android.Runtime.JNIEnv.NewArray
Reviewing, the Android API for `ColorStateList`, the C# binding is:
public unsafe ColorStateList (int[][]? states, int[]? colors)
: base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer)
{
//...
IntPtr native_states = JNIEnv.NewArray (states);
There is a general performance problem in using C# arrays to call into
Java. We have to create a Java array and copy each array element over
to the Java array. We slightly improved upon this scenario in
xamarin/xamarin-android#6870, but the array duplication remains a
problem.
To solve this:
1. Make the `ColorStateList` ctor a "banned API" in C#.
2. Delete `ColorStates.cs`. Create a private `ColorStates` class in Java.
3. Create Java methods for creating `ColorStateList` objects.
I also removed `ColorExtensions.IsOneColor()`, in favor of the pattern:
if (PlatformInterop.CreateEditTextColorStateList(platformPicker.TextColors, textColor.ToPlatform()) is ColorStateList c)
platformPicker.SetTextColor(c);
This reduces the number of JNI calls in this case.
After these changes, we instead get:
2.50ms Microsoft.Maui!Microsoft.Maui.Platform.ColorStateListExtensions.CreateDefault
Which appears to save ~5ms of startup time on a Pixel 5 in `dotnet new
maui`. Other apps that use `Entry`, `CheckBox`, `Switch`, etc. will
also see an improvement.
The only place I didn't fix this is in `TabPage`, as it was not a
straightforward port. Since most apps will be using Shell going
forward anyways, we can revisit this in the future, if needed.
jonathanpeppers
added a commit
that referenced
this pull request
Mar 30, 2022
Context: https://github.com/dotnet/maui/blob/79f2fef9397bbfe2f6aa1527e40f933712a5d2ad/src/Core/src/Platform/Android/ColorStateListExtensions.cs#L50-L51 When running a `dotnet new maui` app, we noticed the log: W monodroid: typemap: failed to map managed type to Java type: System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e (Module ID: 8e4cd939-3275-41c4-968d-d5a4376b35f5; Type token: 33554653) W monodroid-assembly: typemap: called from W monodroid-assembly: at Android.Runtime.JNIEnv.TypemapManagedToJava(Type ) W monodroid-assembly: at Android.Runtime.JNIEnv.GetJniName(Type ) W monodroid-assembly: at Android.Runtime.JNIEnv.FindClass(Type ) W monodroid-assembly: at Android.Runtime.JNIEnv.NewArray(Array , Type ) W monodroid-assembly: at Android.Runtime.JNIEnv.NewArray[Int32[]](Int32[][] ) W monodroid-assembly: at Android.Content.Res.ColorStateList..ctor(Int32[][] , Int32[] ) W monodroid-assembly: at Microsoft.Maui.Platform.ColorStateListExtensions.CreateButton(Int32 enabled, Int32 disabled, Int32 off, Int32 pressed) We see this ~17 times on startup. In this case, the Android binding is: public unsafe ColorStateList (int[][]? states, int[]? colors) : base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer) { //... IntPtr native_states = JNIEnv.NewArray (states); Looking at the stack trace, it seems like this is happening for every item in the array? We could probably call `FindClass(typeof (T))` once, and use it for every item in the array. This would avoid many typemap lookups. Previously this showed up in `dotnet trace` output for a Pixel 5: 6.05ms Mono.Android!Android.Runtime.JNIEnv.NewArray 4.84ms Mono.Android!Android.Runtime.JNIEnv.NewArray(System.Array,System.Type) After these changes: 5.04ms Mono.Android!Android.Runtime.JNIEnv.NewArray 3.83ms Mono.Android!Android.Runtime.JNIEnv.NewArray(System.Array,System.Type,intptr) This change seems to save ~1ms on startup for `dotnet new maui` on a Pixel 5 device.
jonathanpeppers
added a commit
to jonathanpeppers/maui
that referenced
this pull request
Jul 5, 2022
`dotnet trace` output of `dotnet new maui` on a Pixel 5 device shows:
16.63ms microsoft.maui!Microsoft.Maui.Platform.ColorStateListExtensions.CreateDefault(int)
16.63ms mono.android!Android.Content.Res.ColorStateList..ctor(int[][],int[])
11.38ms mono.android!Android.Runtime.JNIEnv.NewArray
Reviewing, the Android API for `ColorStateList`, the C# binding is:
public unsafe ColorStateList (int[][]? states, int[]? colors)
: base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer)
{
//...
IntPtr native_states = JNIEnv.NewArray (states);
There is a general performance problem in using C# arrays to call into
Java. We have to create a Java array and copy each array element over
to the Java array. We slightly improved upon this scenario in
xamarin/xamarin-android#6870, but the array duplication remains a
problem.
To solve this:
1. Make the `ColorStateList` ctor a "banned API" in C#.
2. Delete `ColorStates.cs`. Create a private `ColorStates` class in Java.
3. Create Java methods for creating `ColorStateList` objects.
I also removed `ColorExtensions.IsOneColor()`, in favor of the pattern:
if (PlatformInterop.CreateEditTextColorStateList(platformPicker.TextColors, textColor.ToPlatform()) is ColorStateList c)
platformPicker.SetTextColor(c);
This reduces the number of JNI calls in this case.
After these changes, we instead get:
2.44ms microsoft.maui!Microsoft.Maui.Platform.ColorStateListExtensions.CreateDefault(int)
Which appears to save ~14ms of startup time on a Pixel 5 in `dotnet new
maui`. Other apps that use `Entry`, `CheckBox`, `Switch`, etc. will
also see an improvement.
The only place I didn't fix this is in `TabPage`, as it was not a
straightforward port. Since most apps will be using Shell going
forward anyways, we can revisit this in the future, if needed.
PureWeen
pushed a commit
to dotnet/maui
that referenced
this pull request
Jul 18, 2022
`dotnet trace` output of `dotnet new maui` on a Pixel 5 device shows:
16.63ms microsoft.maui!Microsoft.Maui.Platform.ColorStateListExtensions.CreateDefault(int)
16.63ms mono.android!Android.Content.Res.ColorStateList..ctor(int[][],int[])
11.38ms mono.android!Android.Runtime.JNIEnv.NewArray
Reviewing, the Android API for `ColorStateList`, the C# binding is:
public unsafe ColorStateList (int[][]? states, int[]? colors)
: base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer)
{
//...
IntPtr native_states = JNIEnv.NewArray (states);
There is a general performance problem in using C# arrays to call into
Java. We have to create a Java array and copy each array element over
to the Java array. We slightly improved upon this scenario in
xamarin/xamarin-android#6870, but the array duplication remains a
problem.
To solve this:
1. Make the `ColorStateList` ctor a "banned API" in C#.
2. Delete `ColorStates.cs`. Create a private `ColorStates` class in Java.
3. Create Java methods for creating `ColorStateList` objects.
I also removed `ColorExtensions.IsOneColor()`, in favor of the pattern:
if (PlatformInterop.CreateEditTextColorStateList(platformPicker.TextColors, textColor.ToPlatform()) is ColorStateList c)
platformPicker.SetTextColor(c);
This reduces the number of JNI calls in this case.
After these changes, we instead get:
2.44ms microsoft.maui!Microsoft.Maui.Platform.ColorStateListExtensions.CreateDefault(int)
Which appears to save ~14ms of startup time on a Pixel 5 in `dotnet new
maui`. Other apps that use `Entry`, `CheckBox`, `Switch`, etc. will
also see an improvement.
The only place I didn't fix this is in `TabPage`, as it was not a
straightforward port. Since most apps will be using Shell going
forward anyways, we can revisit this in the future, if needed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: https://github.com/dotnet/maui/blob/79f2fef9397bbfe2f6aa1527e40f933712a5d2ad/src/Core/src/Platform/Android/ColorStateListExtensions.cs#L50-L51
When running a
dotnet new mauiapp, we noticed the log:We see this ~17 times on startup. In this case, the Android binding is:
Looking at the stack trace, it seems like this is happening for every
item in the array? We could probably call
FindClass (typeof (T))once, and use it for every item in the array. This would avoid many
typemap lookups.
Previously this showed up in
dotnet traceoutput for a Pixel 5:After these changes:
This change seems to save ~1ms on startup for
dotnet new mauion aPixel 5 device.