-
Notifications
You must be signed in to change notification settings - Fork 63
[Java.Interop] remove IEnumerable iterators called at startup #555
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
Conversation
|
Probably should wait on the results of the PR build here: dotnet/android#4102 |
6c87c2b to
cc99f0d
Compare
|
This change on the xamarin-android side seems to be OK: https://build.azdo.io/3368801 |
389d5ee to
5eebe96
Compare
|
An updated xamarin-android build is running here: https://build.azdo.io/3380356 |
|
Looks like we'll need to rebase the dotnet/android#4102 PR and rebase atop dotnet/android@e22be491, as it's getting lots of build failures due to |
|
New build is running at: https://build.azdo.io/3381690 |
|
My concern with this is that it's a semantic API break: Currently a subclass would override The After this PR, Now on the "plus" side, (in theory) nobody is actually using this API, so changing semantics shouldn't actually break anybody. That said, it instead results in documentation confusion. We'll have this class: partial class JniTypeManager {
public JniTypeSignature GetTypeSignature (Type type);
protected virtual string GetSimpleReference (Type type);
public IEnumerable<JniTypeSignature> GetTypeSignatures (Type type);
protected virtual IEnumerable<string> GetSimpleReferences (Type type);
}Which method needs to be overridden? One? Both? I think it would be better if protected virtual string GetSimpleReference (Type type)
{
return GetSimpleReferences (type).FirstOrDefault ();
}That way existing code will not require a semantic change, but only a performance change. |
When profiling with `adb setprop debug.mono.profile log:alloc`, I
noticed the following allocations:
Allocation summary
Bytes Count Average Type name
84840 505 168 Java.Interop.JniRuntime.JniTypeManager.<CreateGetTypeSignaturesEnumerator>d__11
24096 502 48 Android.Runtime.AndroidTypeManager.<GetSimpleReferences>d__1
This was using the Xamarin.Forms integration project in xamarin-android:
https://github.com/xamarin/xamarin-android/tree/master/tests/Xamarin.Forms-Performance-Integration
100K of memory allocations are a bit much, so seemed like it might help
to reduce these.
I made a version of both `JniTypeManager.CreateTypeSignatures` and
`AndroidTypeManager.GetSimpleReferences` that return a single value
instead of an `IEnumerable` and `yield return`.
Other general performance changes:
* Added `[MethodImpl (MethodImplOptions.AggressiveInlining)]` to
`JniTypeManager.AssertValid`. It is called quite frequently.
* Removed a call to `.GetTypeInfo()`, as this was not needed. It wraps
`System.Type` in another type that was probably originally used for
netstandard 1.x compatibility.
See: https://github.com/mono/mono/blob/c5b88ec4f323f2bdb7c7d0a595ece28dae66579c/mcs/class/referencesource/mscorlib/system/reflection/introspectionextensions.cs#L24
Changes had to be made in both java.interop and xamarin-android for
this to work.
~~ Results ~~
I was able to see a startup improvement in a Release build of the
Xamarin.Forms integration project on a Pixel 3 XL:
Before:
01-08 14:53:34.926 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +779ms
01-08 14:53:38.892 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +764ms
01-08 14:53:42.859 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +753ms
01-08 14:53:46.795 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +748ms
01-08 14:53:50.792 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +764ms
01-08 14:53:54.759 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +756ms
01-08 14:53:58.726 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +764ms
01-08 14:54:02.675 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +749ms
01-08 14:54:06.691 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +759ms
01-08 14:54:10.641 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +749ms
Average(ms): 758.5
Std Err(ms): 3.0523215208537
Std Dev(ms): 9.65228815704684
After:
01-08 14:55:56.972 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +772ms
01-08 14:56:00.906 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +741ms
01-08 14:56:05.006 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +741ms
01-08 14:56:08.940 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +748ms
01-08 14:56:12.890 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +747ms
01-08 14:56:16.839 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +753ms
01-08 14:56:20.824 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +757ms
01-08 14:56:24.775 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +746ms
01-08 14:56:28.776 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +747ms
01-08 14:56:32.740 1473 1503 I ActivityTaskManager: Displayed Xamarin.Forms_Performance_Integration/xamarin.forms.performance.integration.MainActivity: +748ms
Average(ms): 750
Std Err(ms): 2.87904305089189
Std Dev(ms): 9.10433352249844
It seems like this might save ~8ms on startup?
There is also a good improvement to memory usage/allocations.
Looking at Mono's memory summary:
Before:
Mono System:
Working Set : 71008256
Private Bytes : 601202688
Virtual Bytes : 1306873856
After:
Mono System:
Working Set : 70774784
Private Bytes : 552910848
Virtual Bytes : 1302822912
Then the total GC summary:
Before:
Event: GC allocations : 9340
After:
Event: GC allocations : 8867
Updates based on feedback
* Improved `ArgumentException` to include the type name
* Removed `XA_INTEGRATION` defines
* Code comments
This behavior will be overridden in xamarin-android
5eebe96 to
fdf538e
Compare
|
An updated xamarin-android build is running here: https://build.azdo.io/3384618 |
|
I think that build looks OK, the one failing phase seems unrelated, a bunch of: I sent this to see if it helps: dotnet/android#4124 |
When profiling with
adb setprop debug.mono.profile log:alloc, Inoticed the following allocations:
This was using the Xamarin.Forms integration project in xamarin-android:
https://github.com/xamarin/xamarin-android/tree/master/tests/Xamarin.Forms-Performance-Integration
100K of memory allocations are a bit much, so seemed like it might help
to reduce these.
I made a version of both
JniTypeManager.CreateTypeSignaturesandAndroidTypeManager.GetSimpleReferencesthat return a single valueinstead of an
IEnumerableandyield return.Other general performance changes:
Added
[MethodImpl (MethodImplOptions.AggressiveInlining)]toJniTypeManager.AssertValid. It is called quite frequently.Removed a call to
.GetTypeInfo(), as this was not needed. It wrapsSystem.Typein another type that was probably originally used fornetstandard 1.x compatibility.
See: https://github.com/mono/mono/blob/c5b88ec4f323f2bdb7c7d0a595ece28dae66579c/mcs/class/referencesource/mscorlib/system/reflection/introspectionextensions.cs#L24
Changes had to be made in both java.interop and xamarin-android for
this to work.
Results
I was able to see a startup improvement in a Release build of the
Xamarin.Forms integration project on a Pixel 3 XL:
It seems like this might save ~8ms on startup?
There is also a good improvement to memory usage/allocations.
Looking at Mono's memory summary:
Then the total GC summary: