RFR: 8264288: Performance issue with MethodHandle.asCollector [v3]
jvernee at openjdk.java.net
Mon Apr 5 12:03:08 UTC 2021
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);
> 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
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.
More information about the core-libs-dev