RFR: 8264288: Performance issue with MethodHandle.asCollector [v3]

[Jorn Vernee]

On Fri, 2 Apr 2021 13:23:07 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> That's an elegant solution.
>> 
>> At first i thought it might unduly perturb lambda form generation and caching. but you slotted a different lambda form implementation underneath the varargs implementation.
>
> I've addressed review comments, plus some other things:
> 
> - I realized that I was calling the uncached version of the store function factory. Fixed that.
> - I also realized that there's already an `ARRAY_STORE` intrinsic, which I'm now using to avoid generating a call.
> - I also realized that since we only have 1 array creation handle per lambda form, we can instead generate a direct call to `Array::newInstance` instead of going through the array constructor handle (which also avoids having to use a BoundMethodHandle).
> - Finally, I added an instrinsic, under the old `NEW_ARRAY` name, that intrinsifies a call to `Array::newInstance` if the component type argument is constant (which it is in this case).
> 
> As a result, the lambda form is now fully intrinsified (no more calls in the generated bytecode) e.g.:
> 
> static java.lang.Object collector001_LLLL_L(java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object);
>     Code:
>        0: iconst_3
>        1: anewarray     #12                 // class java/lang/String
>        4: astore        4
>        6: aload         4
>        8: checkcast     #14                 // class "[Ljava/lang/String;"
>       11: dup
>       12: astore        4
>       14: iconst_0
>       15: aload_1
>       16: checkcast     #12                 // class java/lang/String
>       19: aastore
>       20: aload         4
>       22: iconst_1
>       23: aload_2
>       24: checkcast     #12                 // class java/lang/String
>       27: aastore
>       28: aload         4
>       30: iconst_2
>       31: aload_3
>       32: checkcast     #12                 // class java/lang/String
>       35: aastore
>       36: aload         4
>       38: areturn
> 
> Thanks,
> Jorn

Addressed latest review comments:
- Reverted back to using an injected constructor handle (to be able to take advantage of lambda form sharing)
- Added lambda form sharing for empty and reference arrays
- Added a test case for a custom class to VarargsArrayTest, which catches the illegal access error in the intrinsified case that Vlad pointed out (though the itrinsic itself is now removed).

I also had to switch back to the un-cached version for creating the array element setter, since the cached version casts the array to be a specific type, and if the lambda form is shared, this won't work (e.g. casting an Object[] to String[], depending on the order of creating the collectors). Adding caching there is left as a followup.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3306