RFR(S) JDK-8206140 [lworld] Move return value null checks into the callee (original) (raw)

Ioi Lam ioi.lam at oracle.com
Tue Jul 10 15:33:35 UTC 2018

On 7/10/18 5:01 AM, Tobias Hartmann wrote:

Hi Ioi,

thanks for looking at this! On 10.07.2018 07:07, Ioi Lam wrote: http://cr.openjdk.java.net/~iklam/valhalla/8206140lworldnullcheckincallee.v02/ Hi Tobias,

Thanks for the comments. I've updated a new version:

http://cr.openjdk.java.net/~iklam/valhalla/8206140_lworld_null_check_in_callee.v03/

- doCall.cpp: The cast to NOTNULL is only correct if we know that the return value is a value type (which can never be null). Currently, you are always casting to NOTNULL which is not correct.

Fixed. Now it looks like:

 670             const Type*       sig_type = TypeOopPtr::make_from_klass(ctype->as_klass());  671             if (ct == T_VALUETYPE) {  672               // A NULL ValueType cannot be returned to compiled code. The 'areturn' bytecode  673               // handler will deoptimize its caller if it is about to return a NULL ValueType.  674               sig_type = sig_type->join_speculative(TypePtr::NOTNULL);  675             }

- TestLWorld.java: Where is GetNullAsm and RarelyUsedValueUserAsm defined? I added them to the webrev.

To improve interpreter speed (so we won't excessively trap into the VM whenever a null is returned -- this is especially important for methods that are NOT returning a VT), I added 2 bits in Method::flags. These allow the interpreter to quickly check if the areturn is working on a method that returns a VT.

The flags handling is complicated by the fact that "legacy" classes may return a VT (see [1]). Please see my comments around Method::checkreturningvt. I added a new test case for this -- see TestLWorld::test78). Anyway, I am not happy with this check, so if you can think of a better way, please let me know. Great that you were able to create a test for this. I don't think we can easily avoid that check but I think it's sufficient if you add the detailed explanation to the test and remove the else branch (or add a short comment) in method.cpp.

I think it's better to keep the comments in the code, as the test case may get moved or become out of sync with the code. I've added a comment in the test case to point back to the explanation in method.cpp.

Thanks

More information about the valhalla-dev mailing list