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

[Sergey Kuksenko]

Hi All,

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

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