On Fri, 2 Apr 2021 14:41:06 GMT, Jorn Vernee <[email protected]> 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
