[linker] Avoid dictionary creation and lookup in JNI registration #2587

radekdoulik · 2019-01-04T12:05:50Z

Start of implementation of #2586

radekdoulik · 2019-01-04T12:11:06Z

Initial measurements from profiler (5 runs of XF test on Pixel 2 XL)

new:

 Total(ms)  Self(ms)  Calls Method name
 123        0         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
 120        1         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
 121        0         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
 121        1         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
 121        1         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)

old:

 Total(ms)  Self(ms)  Calls Method name
 179        1         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
 178        1         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
 178        1         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
 179        1         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
 180        1         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)

  86        1          1 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:Prefill ()
  85        1          1 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:Prefill ()
  85        1          1 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:Prefill ()
  86        1          1 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:Prefill ()
  87        1          1 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:Prefill ()

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/MonoDroidMarkStep.cs

jonpryor · 2019-01-04T14:31:14Z

What do the numbers in the earlier comment mean? Time in ms?

radekdoulik · 2019-01-04T14:48:36Z

Yeah, the profiler output columns. I have updated the comment to include the headers.

jonpryor · 2019-01-07T10:03:43Z

src/Mono.Android/Android.Runtime/AndroidRuntime.cs

+	                                fixed (char *src = str) {
+	                                        int c;
+	                                        char *s = src;
+	                                        while ((c = s[0]) != 0) {


This doesn't look safe if str is null.

It also appears to require that str be null-terminated to work with strings with str.Length==0. I'm not sure if this is a portable assumption; it's true for mono, but is it true for .NET? Will it be true in the future?

I'd kinda prefer a for loop and removal of the conditional:

int i; int len = str.Length; for (i = 0; (i+1) < len; i += 2) { hash1 = ((hash1 << 5) + hash1) ^ src [i]; hash2 = ((hash2 << 5) + hash2) ^ src [i+1]; } if (i < len) { hash1 = ((hash1 << 5) + hash1) ^ src [i]; }

Yes, this repeats the expression for hash1, but it removes a conditional from the primary loop. It would be interesting to see how the performance compares.

jonpryor · 2019-01-07T10:04:34Z

src/Mono.Android/Android.Runtime/AndroidRuntime.cs

-			{
-				// fill code added by the linker
-			}
+			// should stay in sync with MonoDroidMarkStep.GetStringHashCode


Instead of a comment, why don't we stick this into a dedicated .cs file which is included into both projects, possibly via (new?) shared project? Then we don't need to document that the methods be in sync, as the same source will be used.

jonpryor · 2019-01-07T10:05:06Z

src/Mono.Android/Android.Runtime/AndroidRuntime.cs


 			static MagicRegistrationMap ()
 			{
-				Prefill ();


Do we need this static constructor at all then?

jonpryor · 2019-01-07T10:05:59Z

src/Mono.Android/Android.Runtime/AndroidRuntime.cs

 			{
+				int hashCode;


This variable declaration feels weird. Why is it here?

jonpryor · 2019-01-07T10:10:26Z

As with commits 9be682a and 63b5d6d, we'll want a description of the IL that this PR is generating.

radekdoulik · 2019-01-07T11:22:54Z

@jonpryor, it is work in progress, so I will address many of those things in the final version. BTW, the hash calculation method is taken from csc output disassembly.

radekdoulik · 2019-01-07T11:38:38Z

Actually I used the one from our corlib (for now): https://github.com/mono/mono/blame/master/mcs/class/corlib/ReferenceSources/String.cs#L414-L435

radekdoulik · 2019-01-11T14:27:26Z

The first implementation tested the idea, but used linear search in the list of hash code values.

I have tried 2 approaches to improve that, which led to interesting observations.

Binary search: use unrolled binary search in hash code values, which is similar to

switch (hashCode) {
  case <value0>:
    ...
    break;
  case <value1>:
    ...
    break;
}

Modulo: use hashCode modulo number of hash code values. That looks like:

switch (hashCode % size) {
  case 0:
    ...
    break;
  case 1:
    ...
    break;
}

The results were interesting.

BS:
     145        0         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
     144        0         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
     145        0         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
     146        0         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
     147        0         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)

Mod:
      67        0         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
      67        0         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
      67        0         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
      69        0         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)
      66        0         15 Android.Runtime.AndroidTypeManager:FastRegisterNativeMembers (Java.Interop.JniType,System.Type,string)

It shows how important is the JIT time here. The performance of the new lookup methods are similar (note that CallRegisterMethodByTypeName is called though FastRegisterNativeMembers):

BS:
      15        7         15 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:CallRegisterMethodByTypeName (Java.Interop.JniNativeMethodRegistrationArguments,string)
      15        7         15 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:CallRegisterMethodByTypeName (Java.Interop.JniNativeMethodRegistrationArguments,string)
      15        7         15 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:CallRegisterMethodByTypeName (Java.Interop.JniNativeMethodRegistrationArguments,string)
      16        7         15 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:CallRegisterMethodByTypeName (Java.Interop.JniNativeMethodRegistrationArguments,string)
      15        7         15 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:CallRegisterMethodByTypeName (Java.Interop.JniNativeMethodRegistrationArguments,string)

Mod:
      16        7         15 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:CallRegisterMethodByTypeName (Java.Interop.JniNativeMethodRegistrationArguments,string)
      16        7         15 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:CallRegisterMethodByTypeName (Java.Interop.JniNativeMethodRegistrationArguments,string)
      16        7         15 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:CallRegisterMethodByTypeName (Java.Interop.JniNativeMethodRegistrationArguments,string)
      16        7         15 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:CallRegisterMethodByTypeName (Java.Interop.JniNativeMethodRegistrationArguments,string)
      15        7         15 Android.Runtime.AndroidTypeManager/MagicRegistrationMap:CallRegisterMethodByTypeName (Java.Interop.JniNativeMethodRegistrationArguments,string)

The Mod version of CallRegisterMethodByTypeName is slightly slower, because we end up with more collisions. What's more important here is the length of the CallRegisterMethodByTypeName method. Because the Mod version produces significantly shorter method, the JIT time is supposedly shorter and because the method runs only 15 times during the startup, the price of time spent JITing the method plus 15 runs is high.

So it looks like the time spent in JIT is much more important then I thought.

I plan to look at mono performance counters to see whether I can get the JIT times per method.

It also makes the profiled AOT even more interesting.

radekdoulik · 2019-01-17T18:18:42Z

Idea to consider: what if we generate this part of registration in C instead of IL and link the compiled object file to libmonodroid? Or add it to the object file @grendello plans to use and link on device? (IIRC)

This way we can avoid jitting the code and don't need AOT to be employed.

Maybe we can identify other parts of startup process, which can be implemented in C to speed the app start up?

radekdoulik · 2019-02-05T17:07:35Z

Note: after discussion and JIT times analysis it looks like it would be best to store the indexes to typemap files we use. That should help even before the typemap files are converted to object files.

Another extension of that idea is to store metadata tokens in the typemap for the registration methods. That would make the process a lot simpler and faster.

jonpryor · 2019-02-07T17:19:01Z

Another extension of that idea is to store metadata tokens in the typemap for the registration methods.

While this is a good idea and should be pursued, metadata tokens are a per-assembly construct, while the typemap files will contain types from all assemblies. This will need to be solved.

radekdoulik · 2019-02-08T12:23:30Z

We already have that solved here, as we use the imported references. So the tokens are valid in Mono.Android.dll.

We already have the code like this today, which has token for a registration type for class MainActivity from [xatemplate] assembly. Below that is reference to another class, which is inside of Mono.Android.dll - for comparison. It is part of CallRegisterMethodByIndex method we generate:

	IL_0182: ldtoken [xatemplateaot]xatemplateaot.MainActivity/'__<$>_jni_marshal_methods'
	IL_0187: br IL_053a

	IL_018c: ldtoken Android.Runtime.InputStreamAdapter/'__<$>_jni_marshal_methods'
	IL_0191: br IL_053a

This is prepared in linker, so we already know the references and can import them.

[linker] Avoid dictionary creation and lookup in JNI registration

6143cd3

radekdoulik requested review from dellis1972, jonathanpeppers and jonpryor as code owners January 4, 2019 12:05

radekdoulik added the do-not-merge PR should not be merged. label Jan 4, 2019

dellis1972 reviewed Jan 4, 2019

View reviewed changes

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/MonoDroidMarkStep.cs Show resolved Hide resolved

jonpryor reviewed Jan 7, 2019

View reviewed changes

src/Mono.Android/Android.Runtime/AndroidRuntime.cs

{

int hashCode;

Copy link

Member

jonpryor Jan 7, 2019

Choose a reason for hiding this comment

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

This variable declaration feels weird. Why is it here?

Base automatically changed from master to main March 5, 2021 23:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[linker] Avoid dictionary creation and lookup in JNI registration #2587

[linker] Avoid dictionary creation and lookup in JNI registration #2587

radekdoulik commented Jan 4, 2019

radekdoulik commented Jan 4, 2019 •

edited

Loading

jonpryor commented Jan 4, 2019

radekdoulik commented Jan 4, 2019

jonpryor Jan 7, 2019

jonpryor Jan 7, 2019

jonpryor Jan 7, 2019

jonpryor Jan 7, 2019

jonpryor commented Jan 7, 2019

radekdoulik commented Jan 7, 2019 •

edited

Loading

radekdoulik commented Jan 7, 2019

radekdoulik commented Jan 11, 2019 •

edited

Loading

radekdoulik commented Jan 17, 2019 •

edited

Loading

radekdoulik commented Feb 5, 2019

jonpryor commented Feb 7, 2019

radekdoulik commented Feb 8, 2019 •

edited

Loading

[linker] Avoid dictionary creation and lookup in JNI registration #2587

Are you sure you want to change the base?

[linker] Avoid dictionary creation and lookup in JNI registration #2587

Conversation

radekdoulik commented Jan 4, 2019

radekdoulik commented Jan 4, 2019 • edited Loading

jonpryor commented Jan 4, 2019

radekdoulik commented Jan 4, 2019

jonpryor Jan 7, 2019

Choose a reason for hiding this comment

jonpryor Jan 7, 2019

Choose a reason for hiding this comment

jonpryor Jan 7, 2019

Choose a reason for hiding this comment

jonpryor Jan 7, 2019

Choose a reason for hiding this comment

jonpryor commented Jan 7, 2019

radekdoulik commented Jan 7, 2019 • edited Loading

radekdoulik commented Jan 7, 2019

radekdoulik commented Jan 11, 2019 • edited Loading

radekdoulik commented Jan 17, 2019 • edited Loading

radekdoulik commented Feb 5, 2019

jonpryor commented Feb 7, 2019

radekdoulik commented Feb 8, 2019 • edited Loading

radekdoulik commented Jan 4, 2019 •

edited

Loading

radekdoulik commented Jan 7, 2019 •

edited

Loading

radekdoulik commented Jan 11, 2019 •

edited

Loading

radekdoulik commented Jan 17, 2019 •

edited

Loading

radekdoulik commented Feb 8, 2019 •

edited

Loading