On Fri, 2 Apr 2021 13:23:07 GMT, Jorn Vernee <jver...@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
