RFR: Caching MethodType's descriptor string improves lambda linkage performance

[Christian Thalinger]

On Sep 26, 2013, at 9:22 AM, Sergey Kuksenko <sergey.kuksenko at oracle.com> wrote:

> Hi All,
> 
> I updated fix.
> You may find it here
> http://cr.openjdk.java.net/~skuksenko/jsr335/8024635/webrev.01/

The change looks good.  Since we use Objects.requireNonNull now we can remove the null check comment:

+        Objects.requireNonNull(rtype);  // null check

> 
> See my comments and new webrev descition below
> 
> On 09/18/2013 12:59 AM, John Rose wrote:
>>>> We can tweak that later if necessary.  There are probably only a small number of such strings, so maybe a small leaky map would do the trick as well. 
>>> In case of small amount of such strings we will get a huge overhead from
>>> any kind of map.
>> I would expect O(10^1.5) memory references and branches.  Depends on what you mean by "huge".
> Sure. I think the question to use external map or field may be decided
> later when/if it will be needed.
> Just some statictics, I've collected on nashorn/octane benchmarks
> (statistics collected only for the single(first) benchmark iteration:
> mandreel: 514 unique strings, toMethodDescriptorString called 69047 times
> box2d: 560 unique strings, 34776 toMethodDescriptorString invocations.
> 
>> Would there be any benefit from recording the original string from the constant pool?  The JVM path for this is SystemDictionary::find_method_handle_type, which makes an upcall to MethodHandleNatives.findMethodHandleType.  Currently only the ptypes and rtype are passed, but the JVM could also convert the internal Symbol to a java.lang.String and pass that also.  In that case, MT's created by JVM upcalls would be pre-equipped with descriptor strings.
>> 
>> This is probably worth an experiment, although it requires some JVM work.
> 
> I am definitely sure that we shouldn't do that.
> Right now caching desriptor strings is internal decision of MethodType.
> Otherwise it will make interfaces more complex.
> 
>> I hope you get my overall point:  Hand optimizations have a continuing cost, related to obscuring the logical structure of the code.  The transforming engineer might make a mistake, and later maintaining engineers might also make mistakes.
>> 
>> https://blogs.oracle.com/jrose/entry/three_software_proverbs
> 
> And it doesn't mean that we should afraid any kind of optimization.
> Lucky positions - someone(or something) will optimize it for us. But
> sometimes JIT (even smart JIT) is not enough.
> 
> Let's back to the patch.construstors
> I decided not change original checkPType/checkRType code, except
> explicit Objects.requireNonNull. The problem here that we are creating
> new MethodType objects which are already exists, we have to create MT as
> a key for searching in the internedTable. Ratio between already exiting
> and new MethodTypes a quite huge.
> Here is some statistics I've collected on nashorn/octane benchmarks
> (statistics collected only for the single(first) benchmark iteration:
> 
> mandreel:
>  - 1739 unique MethodTypes,
>  - 878414 MethodType.makeImpl requests;
> box2d:
>  - 1861 unique MethodTypes,
>  - 527486 MethodType.makeImpl requests;
> gameboy:
>  - 2062 unique MethodTypes,
>  - 311378 MethodType.makeImpl requests;
> 
> So
> 1. MethodType constructor do checkPTypes/checkRType which are frequently
> (99%) useless - we already have the same (checked) MethodType in the
> internTable.
> 2. Less than 1% of created MethodTypes will be stored into internTable,
> but reusing that MethodType objects prevent ti from scalar replacement.
> (I've heard that Graal may do flow-sensitive scalar replacement, but C2
> can't do).
> 
> What I did. I separate constructors:
> - for long lived MethodTypes with all checks,
> - for short lived MethodTypes used only as key for searching in the
> InterTable, no checks are performed.
> if we didn't find  MethodType in the table we always create a new one
> (with checks) that is required in less than 1% cases. And we remove at
> least one obstacle for scalar replacement. Unfortunately, scalaer
> replacement for expression "internTable.get(new MethodType(rtype,
> ptypes))" was not performed, the reason may be evaluated, and hope it
> would be easier to achieve scalarization in this case.
> 
> 
> 
> 
> -- 
> Best regards,
> Sergey Kuksenko