[9] RFR [XS] 8054492: Casting can result in redundant null checks in generated code

[Vladimir Kozlov]

Thank you, John

Updated webrev:

http://cr.openjdk.java.net/~kvn/8054492/webrev.03/

On 10/29/14 2:51 PM, John Rose wrote:
> On Oct 29, 2014, at 1:14 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com
> <mailto:vladimir.kozlov at oracle.com>> wrote:
>
>> I hope it is the final version.
>>
>> http://cr.openjdk.java.net/~kvn/8054492/webrev.02/
>>
>> Full blown intrinsic is created for Class.cast(). It tries, first, to
>> fold statically the code. If it can't, it calls gen_checkcast() and
>> generate uncommon trap when the exception have to be thrown. Also if
>> we hit uncommon trap too match it will not generate instrinsic with
>> dynamic checks during recompilation, but it still tries to fold it
>> statically.
>>
>> Since the intrinsic can be skipped I marked Class.cast() as
>> force_inline method to make sure it is inlined to propagate checkcast
>> information in compiled code.
>
> Good.  Suggest a comment like:
>        set_intrinsic_id(id);
> +     if (id == vmIntrinsics::_class_cast) {
> ++     // even if the intrinsic is rejected, we want to inline this
> simple method
> +       set_force_inline(true);
> +     }
>        return;

Comment is added.

>
> Nit:  should be "_Class_cast" not "_class_cast" (cf. _Short_valueOf etc.).

Renamed.

>
> +      // Don't use intrinsic, have to throw ClassCastException.
> Suggest additional comment:
> ++     // If the reference is null, the non-intrinsic bytecode will
> optimize appropriately.

Added.

>
> A similar point probably applies to the !is_loaded case which you handle
> next.
> Suggest just bailing out of the intrinsic for that case, same
> as SSC_always_false.
> That might simplify the code.

Agree.

>
> Good test, although I have a sneaking suspicion that nobody is testing
> corner cases like:
>     int.class.cast(null);  // OK
>     int.class.cast(42);  // CCE int != Integer
> I suggest doing a little informal coverage analysis to make sure your
> new code branches get exercised.

I added new test methods.

Thanks,
Vladimir

>
> It looks good; count me a reviewer.
>
>> An additional improvement could be made to type checks in intrinsics
>> if we use speculative types. I filed RFE:
>> https://bugs.openjdk.java.net/browse/JDK-8062477
>
> Thanks for filing that.
>>
>> I added new regression test and added compile_id to WB nmethod
>> information.
>>
>> Tested jtreg hotspot and jtreg j/l/invoke from jdk, CTW, JPRT,
>> performance.
>>
>> Nashorn's Octane-Box2D and Octane-DeltaBlue shows few percents
>> improvement.
>>
>