Refactor dynamic method names creation

[radekdoulik]

Put the dynamic method name logic in one place. It also removes the
formatted string use and uses InvariantCulture.

It hopefully improves the performance (simpler string operations,
InvariantCulture). The change in performance is so small though, that
I wasn't able to measure it.

We use long counter, because ConstructorBuilder can, in some
cases, create a new constructor per java object instance. This should
be addressed in the future. After that we can probably get back to
int counter.

Also associate the created dynamic methods with
DynamicMethodNameCounter class instead of System.Object

[jonpryor]

This doesn't match our normal formatting guidelines; { should be on the same line as the property name.

However, this should be a property; it should be a method. "Normal" property semantics are that the getter won't return a different value every time it's called. (Yes, DateTime.Now doesn't do that. Yes, it should never have been a property in the first place. We shouldn't repeat the design mistakes of DateTime.Now.)

Since this will return a new value every time it's called, it should be a method.

[radekdoulik]

That is one of the conventions, which doesn't make much sense to me :-)

[jonpryor]

Do we really want constructors and methods to share the same counter?

I'm not even sure what range is "normal" for this counter, other than that the counter will increment for every constructor invocation and method registered.

Also note that -- at present -- the ConstructorBuilder count increments every time TypeManager.n_Activate() is invoked. It's not incremented once per method signature; it's incremented once per invocation. This is entirely unbound, and it's entirely imaginable to have Java code construct ~4 billion objects over the course of a (very) long-lived process, which would cause an overflow to occur.

[jonpryor]

...that said, we still have that possible overflow problem now, because of PR #1872...and I didn't think of this scenario before. Doh! :-(

[radekdoulik]

Is that really true? My understanding is that it is called once per type at most (many types don't have <init> constructors) and not per instance.

[jonpryor]

Is that really true?

Let's start with Java Callable Wrappers:

    public HelloAndroid ()
    {
        super ();
        if (getClass () == HelloAndroid.class)
            mono.android.TypeManager.Activate (
                "Mono.Samples.HelloWorld.HelloAndroid, HelloWorld, Version=1.0.0.0, 
                Culture=neutral, PublicKeyToken=null", "", this, new java.lang.Object[] {  });
    }

TypeManager.Activate() is TypeManager.n_Activate(), and is invoked separately for every constructor invocation:

// ~~~ From Java ~~~

new HelloAndroid();
// One TypeManager.n_Activate() invocation

new HelloAndroid();
// Two TypeManager.n_Activate() invocations

new HelloAndroid();
// Three TypeManager.n_Activate() invocations

...

Which brings us to the ConstructorBuilder.CreateDelegate() invocation, for which nothing is cached. (We should absolutely add some caching; it would have noticeable improvements in instance construction! However, we don't currently have any such caching.)

Which is why I said:

the ConstructorBuilder count increments every time TypeManager.n_Activate() is invoked.

I don't see any other interpretation.

[radekdoulik]

Interesting, I will need to take look at it. Hopefully we can address that in #2022.

Meanwhile I have updated the patch so, that we get back to using Guid once we create int.MaxValue methods. I am curious whether it would even be possible to associate that many methods with a type :-) Didn't have time to try it yet.

[jonpryor]

I am curious whether it would even be possible to associate that many methods with a type :-)

It isn't possible to associate int.MaxValue methods with a type; the Java .class file format only permits ushort.MaxValue values within the "constant pool" of a .class file, and each method declared within a Java type must have one or more entries in the constant pool.

However, dynamicMethodNameCounter isn't "scoped" to a single type; it's used across all types.

If it were limited to just methods and not constructors, this means the developer would need to register int.MaxValue/ushort.MaxValue=32768 types in order to overflow the counter.

This is unlikely. :-)

The problem, again, is constructors: constructors are not per-type, they're per-invocation. If Java had an equivalent to ConstructorInfo.Invoke(object, object[]) -- which perhaps might not exist in Java, but let's go with it -- you would only need to initialize one type, and one instance, and just call that one method int.MaxValue times.

That's excessive, but seems considerably more likely than having 32768 types in a .apk.

[radekdoulik]

I mean the type on the managed side here, where the methods/constructors are associated to DynamicMethodCounter class (or System.Object as before).

[jonpryor]

I'm not thrilled with any use of Guid.

Would it be possible to separate constructor from method counting? We "know" -- or are reasonably sure -- that it's highly unlikely that methods will cause an overflow. Constructors can...maybe. At least, I find it harder to convince myself that Constructors cannot cause an overflow.

I'm inclined to think we should split out method counts from constructor counts.

...and perhaps Constructors should use a long? Guid is really, really, slow. Interlocked.Increment(ref long) would still be faster, by many orders of magnitude.

[radekdoulik]

Or just have long here and keep one counter?

[jonpryor]

Using a long would probably be fine, but I suspect -- entirely untested! -- that Interlocked.Increment() on a long will be slower than an int on 32-bit hardware.

Then again, how much longer will 32-bit Android devices be a priority?

[radekdoulik]

OK, updated to use long counter. Hopefully we can fix ConstructorBuilder soon and get back to int.