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

[Vladimir Ivanov]

On Fri, 2 Apr 2021 14:41:06 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 874:
>> 
>>> 872:                 case NEW_ARRAY:
>>> 873:                     Class<?> rtype = name.function.methodType().returnType();
>>> 874:                     if (isStaticallyNameable(rtype)) {
>> 
>> Why do you remote `isStaticallyNameable(rtype)` check? 
>> 
>> It is there for a reason: only a very limited set of classes can be directly referenced from LF bytecode.
>
> I removed the `NEW_ARRAY` intrinsic altogether at first, but added it back in the latest commit. I didn't add the check since I was not aware of the restriction (looks like some of the tests failed as well).
> 
> Good to know, will add a check.

Also, please, add a test case for such scenario. It should be trivial: just use a class defined by the test (loaded by `AppClassLoader`).

>> src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1897:
>> 
>>> 1895:         // field of the receiver handle.
>>> 1896:         // This is left as a followup enhancement, as it needs to be investigated if this causes
>>> 1897:         // profile pollution.
>> 
>> Profile pollution shouldn't be a problem here: both types (element type + array type) will be constants attached to the "receiver" BMH instance and during inlining will be fetched from there.  
>> 
>> I'm fine with handling it as a separate RFE.
>
> That's what I thought as well (but not 100% sure). I can partially roll back the last commit so the code still uses an injected array constructor handle, and then it should be easy to add caching in the cases where the component type is a reference type.

Whatever you prefer. I'm fine with it either way.

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

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