This is Xamarin's bug tracking system. For product support, please use the support links listed in your Xamarin Account.
Bug 32126 - Xamarin.Android call performance is 4 times slower on inherited views
Summary: Xamarin.Android call performance is 4 times slower on inherited views
Status: RESOLVED FIXED
Alias: None
Product: Android
Classification: Xamarin
Component: General (show other bugs)
Version: 5.1
Hardware: PC Windows
: Normal enhancement
Target Milestone: master
Assignee: Jonathan Pryor
URL:
: 34601 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-07-19 21:20 UTC by Jerome Laban
Modified: 2016-02-01 17:50 UTC (History)
2 users (show)

See Also:
Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Perf issue repro (11.89 KB, application/x-zip-compressed)
2015-07-19 21:20 UTC, Jerome Laban
Details

Description Jerome Laban 2015-07-19 21:20:46 UTC
Created attachment 12106 [details]
Perf issue repro

Using Xamarin.Android 5.1.3.1 from VS2015 and the attached sample project, you'll notice that creating a Android.Views.View is approximately 4 times faster (on a very fast device) than creating a class that inherits from Android.Views.View.

In the case of a inherited view, this performance issue is mainly related to the fact in the Android.Views.View binding, if the Java.Lang.Object.ThresholdClass is not of the java type, the constructor does not cache the multiple calls to FindMethod and GetMethodID, unlike in a non-inherited class.

This caching problem is present for both instance creation and member method invocation.
Comment 1 Jonathan Pryor 2015-07-20 12:10:40 UTC
This is addressable via Java.Interop.JniPeerMembers, which sadly is nowhere near ready for release or use. Migrating to a better (caching) object model is a long-term project.
Comment 2 Jerome Laban 2015-08-02 23:37:27 UTC
Actually, I've been fiddling with both invocations, creating the native instances manually and it works just fine. It's actually even faster because even when the threshold class of the java type, there's still a method resolution that is not cached.

Why would caching the native methods and class be any different when the Threshold class is not of the Java type ?

Both are mutually exclusive, so this may not be a memory concern...
Comment 3 Jonathan Pryor 2015-08-03 00:50:32 UTC
> Why would caching the native methods and class be any different when the
> Threshold class is not of the Java type ?

See the Binding Virtual Methods section in the Working with JNI document:

http://developer.xamarin.com/guides/android/advanced_topics/java_integration_overview/working_with_jni/#Binding_Virtual_Methods

In particular, the Binding Implementation section:

    public virtual int Add (int a, int b)
    {
        if (id_add == IntPtr.Zero)
            id_add = JNIEnv.GetMethodID (class_ref, "add", "(II)I");
        if (GetType () == ThresholdType)
            return JNIEnv.CallIntMethod (Handle, id_add, new JValue (a),
                    new JValue (b));
        return JNIEnv.CallNonvirtualIntMethod (Handle,
                ThresholdClass, id_add, new JValue (a), new JValue (b));
    }

Note: the above is *wrong*. (I really should update that doc...)

The bug is the use of `id_add` in the JNIEnv.CallNonvirtualIntMethod() invocation.

The scenario for the bug is this Java code:

    public class AnotherAdder : mono.android.test.Adder {
        public int add (int a, int b) {
            return (a*2) + (b*2);
        }
    }

Then, you bind AnotherAdder. (More likely, you use an Android Binding Project to bind AnotherAdder.)

    [Register (...)]
    public partial class AnotherAdder : Adder {
        // Note: NO override for Add().
    }

Then, you subclass AnotherAdder from C#:

    class MyNewAdder : AnotherAdder {
    }

To sum up, we now have the C# Hierarchy of:

    MyNewAdder < AnotherAdder < Adder

For that hierarchy, there is only one Add() method binding: Adder.Add().

Meanwhile, on the Java side of things, there are *two* add() implementations: Adder.add() and AnotherAdder.add().

With that in mind, what do we expect to happen with:

    var adder = new MyNewAdder();
    adder.Add(1, 2);

MyNewAdder doesn't override Add(), and it subclasses AnotherAdder, so it *should* hit AnotherAdder.add().

Let us now return to the broken binding code:

    public virtual int Add (int a, int b)
    {
        if (id_add == IntPtr.Zero)
            id_add = JNIEnv.GetMethodID (class_ref, "add", "(II)I");
        if (GetType () == ThresholdType)
            return JNIEnv.CallIntMethod (Handle, id_add, new JValue (a),
                    new JValue (b));
        return JNIEnv.CallNonvirtualIntMethod (Handle,
                ThresholdClass, id_add, new JValue (a), new JValue (b));
    }

For the above class hierarchy, ThresholdType is typeof(AnotherAdder), which isn't equal to GetType() (which will be typeof(MyNewAdder)). Consequently, we hit the "super invocation" path:

        return JNIEnv.CallNonvirtualIntMethod (Handle,
                ThresholdClass, id_add, new JValue (a), new JValue (b));

What's `id_add`? It's the jmethodID for Adder.add(), *not* AnotherAdder.add(). Meaning that the type that `id_add` came from differs from ThresholdClass (which is the `jclass` value corresponding to ThresholdType).

However, that doesn't match the CallNonvirtualIntMethod() documentation:

http://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#CallNonvirtual_type_Method_routines

> The methodID argument must be obtained by calling GetMethodID() on the class clazz.

WHICH IS NOT WHAT WAS DONE. `clazz` -- ThresholdClass -- *differed* from the class that was used to find id_add.

As it happens, this pattern used to work Just Fine in Dalvik, which is why that was previously generated and why the documentation uses that pattern. It does NOT work on ART, which implements the JNI spec more correctly.

Meaning that when running on ART, the above broken binding code would invoke the *wrong* method -- it would invoke Adder.add(), NOT AnotherAdder.add(), resulting in "bizarre" behavior -- Bug #17630.

This is bad™. :-)

A fix is to do what we've done: ensure that the jmethodID we pass to JNIEnv.CallNonvirtual*Method() corresponds to ThresholdClass, thus ensuring that the correct method will actually be invoked, and The Right Thing™  happens.

The problem with this strategy is that jmethodIDs aren't cached, in large part because under the current ABI there's nowhere to cache them: the method IDs need to be (re-)resolved separately for each derived type, and the goal for the binding ABI was that it would be thread-safe and require no locking. (This goal may have been a mistake, but that was the original goal.)

Constructors hit the same problem: there's nowhere in the ABI to cache jmethodIDs for the constructor in a location that doesn't involve locking of some sort.

(Another possible solution would be to emit an AnotherAdder.Add() method override, except that's not actually a solution because (1) jart2xml often won't report the add() override, and (2) the Java type could always be modified after-the-fact to add the method override, and we don't want the Binding assembly to be tied to a specific *version* of the Java code, just tied the Java API.)

(Another possible solution would be to just add locking, which requires some additional investigation and profiling.)
Comment 4 Jerome Laban 2015-08-03 06:49:20 UTC
Thanks for the *very* detailed response, that makes perfect sense! 

About the locking performance, I'd probably choose a static Dictionary<T, U> lookup at every call (even Monitor locked) over a FindClass/GetMethodID that calls all sorts of very expensive interop methods and md5 hashing.

Also, in the same way you're generating Java binding classes, would it be possible to generate a C# based registry of non threshold classes as static generic types ? That would let mono do the locking as part of choosing the storage for a static field in a generic class, and use it to back the caching of Java methods.
Comment 5 Jonathan Pryor 2015-08-06 17:11:50 UTC
> would it be possible to generate a C# based registry of non threshold classes as static
> generic types

What?
Comment 6 Jerome Laban 2015-09-16 15:37:19 UTC
Sorry, it does not make much sense, generating code in this context is not useful, the type would not be explicitly available at compile-time to be used.

I still stand by my original comment, and the use of a dictionary vs. resolving the class/method at every single call. The original issue you mentioned was fixed, but that had the effect of significantly degraded the performance of all the bindings.
Comment 9 Jonathan Pryor 2015-10-06 17:19:10 UTC
*** Bug 34601 has been marked as a duplicate of this bug. ***
Comment 10 Jonathan Pryor 2016-02-01 17:50:21 UTC
Partially fixed in monodroid/cb49134f. This should be included in cycle 7.

The solution is to (finally!) implement Comment #1: pull in my skunkworks research project of Java.Interop.dll and use Java.Interop.JniPeerMembers for member invocation. JniPeerMembers has thread-safe caching of jmethodIDs (and more) keyed off runtime types, so jmethodIDs of non-Threshold classes can be properly cached and reused. This is done for both methods and constructors alike.

However, for now the fix will only be in Mono.Android.dll and will not be used in any binding assemblies (as we nail down how all of this works together). Plus, the New World Order won't work on previous versions, so even once I am confident enough to enable this in all binding projects you might not want to use it, depending on which Xamarin.Android versions you wish to support...

Note You need to log in before you can comment on or make changes to this bug.