On Fri, 2 Apr 2021 14:41:06 GMT, Jorn Vernee <jver...@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

Reply via email to