On Mar 14, 2014, at 1:19 PM, Vladimir Ivanov <vladimir.x.ivanov@oracle.com> wrote:

FYI, this change isn't limited to only bytecode assembly improvements, but also contains caching of lambda forms for setters/getter of typed arrays.

Do you mean for MethodHandles.arrayElementGetter/Setter? If so i don't see relevant changes in: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.00/src/share/classes/jav... to say MethodHandleImpl.ArrayAccessor: static final class ArrayAccessor { /// Support for array element access static final HashMap<Class<?>, MethodHandle> GETTER_CACHE = new HashMap<>(); // TODO use it static final HashMap<Class<?>, MethodHandle> SETTER_CACHE = new HashMap<>(); // TODO use it Paul.

If there are any objections, I can back the caching logic out and include it into one of upcoming changes.

Best regards, Vladimir Ivanov

On 3/14/14 3:45 PM, Vladimir Ivanov wrote:

...

http://cr.openjdk.java.net/~vlivanov/8037209/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8037209 440 lines changed: 313 ins; 67 del; 60 mod

This is a cleanup of JSR292 code to improve bytecode assembly code for lambda forms.

Contributed-by: john.r.rose@oracle.com

Testing: jdk/java/{lang/invoke,util}, vm.mlvm.testlist, nashorn, jruby

Configs: -ea -esa -Xverify:all -D...COMPILE_THRESHOLD={0,30} -D...PROFILE_LEVEL={0,1}

Best regards, Vladimir Ivanov _______________________________________________ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

To get into this faster it would be nice if the new private fields (or the existing ones for that matter) had a comment describing what they were for, e.g. + private final byte[] localTypes; + private final Class<?>[] localClasses; I can figure it out from the code, but it would have been a better starting point. If that there are new apparent performance regressions caused by, e.g. profile pollusion from reusing existing method handles, I am fine with this. +1 /M On 14 Mar 2014, at 14:17, Vladimir Ivanov <vladimir.x.ivanov@oracle.com> wrote:

Paul,

You are looking at the other fix (8037210). The correct link is [1].

Best regards, Vladimir Ivanov

[1] http://cr.openjdk.java.net/~vlivanov/8037209/webrev.00/src/share/classes/jav...

On 3/14/14 4:38 PM, Paul Sandoz wrote:

...

On Mar 14, 2014, at 1:19 PM, Vladimir Ivanov <vladimir.x.ivanov@oracle.com <mailto:vladimir.x.ivanov@oracle.com>> wrote:

...

FYI, this change isn't limited to only bytecode assembly improvements, but also contains caching of lambda forms for setters/getter of typed arrays.

Do you mean for MethodHandles.arrayElementGetter/Setter? If so i don't see relevant changes in:

http://cr.openjdk.java.net/~vlivanov/8037210/webrev.00/src/share/classes/jav...

to say MethodHandleImpl.ArrayAccessor:

static final class ArrayAccessor { /// Support for array element access static final HashMap<Class<?>, MethodHandle> GETTER_CACHE = new HashMap<>(); // TODO use it static final HashMap<Class<?>, MethodHandle> SETTER_CACHE = new HashMap<>(); // TODO use it

Paul.

...

If there are any objections, I can back the caching logic out and include it into one of upcoming changes.

Best regards, Vladimir Ivanov

On 3/14/14 3:45 PM, Vladimir Ivanov wrote:

...

http://cr.openjdk.java.net/~vlivanov/8037209/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8037209 440 lines changed: 313 ins; 67 del; 60 mod

This is a cleanup of JSR292 code to improve bytecode assembly code for lambda forms.

Contributed-by: john.r.rose@oracle.com

Testing: jdk/java/{lang/invoke,util}, vm.mlvm.testlist, nashorn, jruby

Configs: -ea -esa -Xverify:all -D...COMPILE_THRESHOLD={0,30} -D...PROFILE_LEVEL={0,1}

Best regards, Vladimir Ivanov _______________________________________________ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

_______________________________________________ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Doh! crossed webrevs, thanks.

Just had a quick look, this looks like a really nice improvement to the array setter/getter support, definitely simplified. IIUC the mh.viewAsType will now handle the appropriate casting. I believe it might reduce the "ceremony" for array setter/getter MHs [1].

I see there is a PROFILE_LEVEL option, by default set to 0, that results in casts not being emitted:

+ if (VerifyType.isNullConversion(Object.class, pclass, false)) { + if (PROFILE_LEVEL > 0) + emitReferenceCast(Object.class, arg); + return; + } ... + mv.visitLdcInsn(constantPlaceholder(cls)); + mv.visitTypeInsn(Opcodes.CHECKCAST, CLS); + mv.visitInsn(Opcodes.SWAP); + mv.visitMethodInsn(Opcodes.INVOKESTATIC, MHI, "castReference", CLL_SIG, false); + if (Object[].class.isAssignableFrom(cls)) + mv.visitTypeInsn(Opcodes.CHECKCAST, OBJARY); + else if (PROFILE_LEVEL > 0) + mv.visitTypeInsn(Opcodes.CHECKCAST, OBJ);

Can you explain a bit the rational for that? These casts are redundant - they aren't required for bytecode correctness. The idea behind PROFILE_LEVEL is to provide more type information to JIT-compiler. Right now, type profiling occurs on every checkcast instruction. So, having these additional instructions we can feed C2 with more accurate information about types.

Consider this as a hack to overcome some of the limitations of current profiling implementation in VM. Best regards, Vladimir Ivanov

Paul.

[1] Below is an inlining trace for an array getter MH in the current code i obtained a few weeks ago when investigating CAS-based MHs for arrays:

Inlining _isInstance on constant Class java/lang/Class Inlining _isInstance on constant Class [Ljava/lang/Object; Inlining _isInstance on constant Class [Ljava/lang/String; Inlining _isInstance on constant Class java/lang/String @ 12 unsafe.ArrayMHTest::get_mh_orig (19 bytes) inline (hot) @ 12 java.lang.invoke.LambdaForm$MH/924874137::invokeExact_MT (15 bytes) inline (hot) @ 2 java.lang.invoke.Invokers::checkExactType (30 bytes) inline (hot) @ 11 java.lang.invoke.MethodHandle::type (5 bytes) accessor @ 11 java.lang.invoke.LambdaForm$MH/1980556943::convert (21 bytes) inline (hot) @ 7 java.lang.invoke.LambdaForm$MH/1892887290::getElement (20 bytes) inline (hot) @ 16 java.lang.invoke.LambdaForm$MH/816943783::convert (37 bytes) inline (hot) @ 6 java.lang.invoke.LambdaForm$BMH/1350345087::reinvoke (26 bytes) inline (hot) @ 12 java.lang.invoke.BoundMethodHandle$Species_LL::reinvokerTarget (8 bytes) inline (hot) @ 22 java.lang.invoke.LambdaForm$DMH/1162873666::invokeStatic_LL_L (15 bytes) inline (hot) @ 1 java.lang.invoke.DirectMethodHandle::internalMemberName (8 bytes) inline (hot) @ 11 sun.invoke.util.ValueConversions::castReference (20 bytes) inline (hot) @ 6 java.lang.Class::isInstance (0 bytes) (intrinsic) @ 17 java.lang.invoke.LambdaForm$BMH/1350345087::reinvoke (26 bytes) inline (hot) @ 12 java.lang.invoke.BoundMethodHandle$Species_LL::reinvokerTarget (8 bytes) inline (hot) @ 22 java.lang.invoke.LambdaForm$DMH/1162873666::invokeStatic_LL_L (15 bytes) inline (hot) @ 1 java.lang.invoke.DirectMethodHandle::internalMemberName (8 bytes) inline (hot) @ 11 sun.invoke.util.ValueConversions::castReference (20 bytes) inline (hot) @ 6 java.lang.Class::isInstance (0 bytes) (intrinsic) @ 33 java.lang.invoke.MethodHandleImpl$ArrayAccessor::getElementL (10 bytes) inline (hot) @ 2 java.lang.Class::cast (27 bytes) inline (hot) @ 6 java.lang.Class::isInstance (0 bytes) (intrinsic) @ 17 java.lang.invoke.LambdaForm$MH/91466391::convert (23 bytes) inline (hot) @ 6 java.lang.invoke.LambdaForm$BMH/1350345087::reinvoke (26 bytes) inline (hot) @ 12 java.lang.invoke.BoundMethodHandle$Species_LL::reinvokerTarget (8 bytes) inline (hot) @ 22 java.lang.invoke.LambdaForm$DMH/1162873666::invokeStatic_LL_L (15 bytes) inline (hot) @ 1 java.lang.invoke.DirectMethodHandle::internalMemberName (8 bytes) inline (hot) @ 11 sun.invoke.util.ValueConversions::castReference (20 bytes) inline (hot) @ 6 java.lang.Class::isInstance (0 bytes) (intrinsic) @ 19 java.lang.invoke.LambdaForm$MH/349834280::identity (10 bytes) inline (hot) @ 6 java.lang.invoke.LambdaForm$DMH/1357006072::invokeStatic_L_L (14 bytes) inline (hot) @ 1 java.lang.invoke.DirectMethodHandle::internalMemberName (8 bytes) inline (hot) @ 10 sun.invoke.util.ValueConversions::identity (2 bytes) inline (hot)

I'd like to revive this review thread. Updated version: http://cr.openjdk.java.net/~vlivanov/8037209/webrev.04/ Paul, I'd like to address your case (if possible) separately. Right now, I don't see any way to avoid null check you see with your tests. Best regards, Vladimir Ivanov On 3/24/14 8:33 PM, Vladimir Ivanov wrote:

Updated version: http://cr.openjdk.java.net/~vlivanov/8037209/webrev.01/

Changes: - rebased to enum-based BasicType; - decided to integrate changes for typed array getters/setters separately;

Best regards, Vladimir Ivanov

On 3/14/14 8:36 PM, Vladimir Ivanov wrote:

...

...

Doh! crossed webrevs, thanks.

Just had a quick look, this looks like a really nice improvement to the array setter/getter support, definitely simplified. IIUC the mh.viewAsType will now handle the appropriate casting. I believe it might reduce the "ceremony" for array setter/getter MHs [1].

I see there is a PROFILE_LEVEL option, by default set to 0, that results in casts not being emitted:

+ if (VerifyType.isNullConversion(Object.class, pclass, false)) { + if (PROFILE_LEVEL > 0) + emitReferenceCast(Object.class, arg); + return; + } ... + mv.visitLdcInsn(constantPlaceholder(cls)); + mv.visitTypeInsn(Opcodes.CHECKCAST, CLS); + mv.visitInsn(Opcodes.SWAP); + mv.visitMethodInsn(Opcodes.INVOKESTATIC, MHI, "castReference", CLL_SIG, false); + if (Object[].class.isAssignableFrom(cls)) + mv.visitTypeInsn(Opcodes.CHECKCAST, OBJARY); + else if (PROFILE_LEVEL > 0) + mv.visitTypeInsn(Opcodes.CHECKCAST, OBJ);

Can you explain a bit the rational for that? These casts are redundant - they aren't required for bytecode correctness. The idea behind PROFILE_LEVEL is to provide more type information to JIT-compiler. Right now, type profiling occurs on every checkcast instruction. So, having these additional instructions we can feed C2 with more accurate information about types.

Consider this as a hack to overcome some of the limitations of current profiling implementation in VM.

Best regards, Vladimir Ivanov

...

Paul.

[1] Below is an inlining trace for an array getter MH in the current code i obtained a few weeks ago when investigating CAS-based MHs for arrays:

Inlining _isInstance on constant Class java/lang/Class Inlining _isInstance on constant Class [Ljava/lang/Object; Inlining _isInstance on constant Class [Ljava/lang/String; Inlining _isInstance on constant Class java/lang/String @ 12 unsafe.ArrayMHTest::get_mh_orig (19 bytes) inline (hot) @ 12 java.lang.invoke.LambdaForm$MH/924874137::invokeExact_MT (15 bytes) inline (hot) @ 2 java.lang.invoke.Invokers::checkExactType (30 bytes) inline (hot) @ 11 java.lang.invoke.MethodHandle::type (5 bytes) accessor @ 11 java.lang.invoke.LambdaForm$MH/1980556943::convert (21 bytes) inline (hot) @ 7 java.lang.invoke.LambdaForm$MH/1892887290::getElement (20 bytes) inline (hot) @ 16 java.lang.invoke.LambdaForm$MH/816943783::convert (37 bytes) inline (hot) @ 6 java.lang.invoke.LambdaForm$BMH/1350345087::reinvoke (26 bytes) inline (hot) @ 12 java.lang.invoke.BoundMethodHandle$Species_LL::reinvokerTarget (8 bytes) inline (hot) @ 22 java.lang.invoke.LambdaForm$DMH/1162873666::invokeStatic_LL_L (15 bytes) inline (hot) @ 1 java.lang.invoke.DirectMethodHandle::internalMemberName (8 bytes) inline (hot) @ 11 sun.invoke.util.ValueConversions::castReference (20 bytes) inline (hot) @ 6 java.lang.Class::isInstance (0 bytes) (intrinsic) @ 17 java.lang.invoke.LambdaForm$BMH/1350345087::reinvoke (26 bytes) inline (hot) @ 12 java.lang.invoke.BoundMethodHandle$Species_LL::reinvokerTarget (8 bytes) inline (hot) @ 22 java.lang.invoke.LambdaForm$DMH/1162873666::invokeStatic_LL_L (15 bytes) inline (hot) @ 1 java.lang.invoke.DirectMethodHandle::internalMemberName (8 bytes) inline (hot) @ 11 sun.invoke.util.ValueConversions::castReference (20 bytes) inline (hot) @ 6 java.lang.Class::isInstance (0 bytes) (intrinsic) @ 33 java.lang.invoke.MethodHandleImpl$ArrayAccessor::getElementL (10 bytes) inline (hot) @ 2 java.lang.Class::cast (27 bytes) inline (hot) @ 6 java.lang.Class::isInstance (0 bytes) (intrinsic) @ 17 java.lang.invoke.LambdaForm$MH/91466391::convert (23 bytes) inline (hot) @ 6 java.lang.invoke.LambdaForm$BMH/1350345087::reinvoke (26 bytes) inline (hot) @ 12 java.lang.invoke.BoundMethodHandle$Species_LL::reinvokerTarget (8 bytes) inline (hot) @ 22 java.lang.invoke.LambdaForm$DMH/1162873666::invokeStatic_LL_L (15 bytes) inline (hot) @ 1 java.lang.invoke.DirectMethodHandle::internalMemberName (8 bytes) inline (hot) @ 11 sun.invoke.util.ValueConversions::castReference (20 bytes) inline (hot) @ 6 java.lang.Class::isInstance (0 bytes) (intrinsic) @ 19 java.lang.invoke.LambdaForm$MH/349834280::identity (10 bytes) inline (hot) @ 6 java.lang.invoke.LambdaForm$DMH/1357006072::invokeStatic_L_L (14 bytes) inline (hot) @ 1 java.lang.invoke.DirectMethodHandle::internalMemberName (8 bytes) inline (hot) @ 10 sun.invoke.util.ValueConversions::identity (2 bytes) inline (hot)

Regarding the extra cast in accessor logic that Paul picked up on: That may be a left-over from obsolete versions of the code, or it may cover for some corner cases, or it could possibly be a re-assurance to the JIT. Generally speaking, we lean heavily on MH types to guarantee a priori correctness of argument types. Paul is right that the stored value type is already validated and (except for corner cases we may be neglecting) does not need re-validation via a cast. It might be an interesting experiment to replace the cast with an assert. Sometimes corner cases bite you. For example, an array store check is necessary, if the type is an interface, because interfaces are weakly checked all the way up to aastore or invokeinterface. Sometimes the JIT cannot see the type assurances implicit in a MH type, and so (when choosing internal MH code shapes) we must choose between handing the JIT code that is not locally verifiable, or adding "reassurance" casts to repair the local verifiability of the code. If the JIT thinks it sees badly-typed code, it might bail out.Note that "locality" of verifiability is a fluid concept, depending sometime on vagaries of inlining decisions. This is the reason for some awkward looking "belt and suspenders" MH internals, such as the free use of casts in LF bytecode rendering. Usually, redundant type verifications (including casts and signatures of incoming arguments) are eliminated, but they can cause (as noted) an extra null check. In theory, that should fold up also, if the null value is replaced by "another" null, as (p == null ? null : identityFunction(p)). — John On Jul 8, 2014, at 3:09 AM, Vladimir Ivanov <vladimir.x.ivanov@oracle.com> wrote:

I'd like to revive this review thread.

Updated version: http://cr.openjdk.java.net/~vlivanov/8037209/webrev.04/

Paul, I'd like to address your case (if possible) separately. Right now, I don't see any way to avoid null check you see with your tests.

On Apr 1, 2014, at 8:29 AM, Paul Sandoz <paul.sandoz@oracle.com> wrote:

On Apr 1, 2014, at 4:10 PM, Vladimir Ivanov <vladimir.x.ivanov@oracle.com> wrote:

...

Paul,

Unfortunately, additional profiling doesn't work for Accessor.checkCast case. The problem is Accessor.checkCast is called from multiple places, so type profile is very likely to be polluted. And it kills the benefits.

So is there any point in doing such a cast in this case?

The type performing the cast is the field type declared as a parameter in the MethodType of the MethodHandle and also held by the Accessor.

IIUC the Invokers.checkExactType should ensure no "unsavoury" instances of the wrong type gets through? (the holder instance is only checked for null, via checkBase).

On Jul 8, 2014, at 9:09 PM, John Rose <john.r.rose@oracle.com> wrote:

Regarding the extra cast in accessor logic that Paul picked up on: That may be a left-over from obsolete versions of the code, or it may cover for some corner cases, or it could possibly be a re-assurance to the JIT.

Generally speaking, we lean heavily on MH types to guarantee a priori correctness of argument types. Paul is right that the stored value type is already validated and (except for corner cases we may be neglecting) does not need re-validation via a cast.

It might be an interesting experiment to replace the cast with an assert.

I did briefly look at that a few months ago, i would need to systematically revisit. My memory is hazy, I seem to recall removing casts perturbed the compilation process more than i expected.

Sometimes corner cases bite you. For example, an array store check is necessary, if the type is an interface, because interfaces are weakly checked all the way up to aastore or invokeinterface.

Sometimes the JIT cannot see the type assurances implicit in a MH type, and so (when choosing internal MH code shapes) we must choose between handing the JIT code that is not locally verifiable, or adding "reassurance" casts to repair the local verifiability of the code. If the JIT thinks it sees badly-typed code, it might bail out.Note that "locality" of verifiability is a fluid concept, depending sometime on vagaries of inlining decisions. This is the reason for some awkward looking "belt and suspenders" MH internals, such as the free use of casts in LF bytecode rendering.

Usually, redundant type verifications (including casts and signatures of incoming arguments) are eliminated, but they can cause (as noted) an extra null check. In theory, that should fold up also, if the null value is replaced by "another" null, as (p == null ? null : identityFunction(p)).

I quickly verified the fold up does as you expect. Also, if i do the following the null check goes away: public class A { volatile String a = "A"; volatile String snull = null; public volatile String b; static final MethodHandle b_setter; static { try { b_setter = MethodHandles.lookup().findSetter(A.class, "b", String.class); } catch (Exception e) { throw new Error(e); } } public static void main(String[] args) { A a = new A(); a.testLoop(); } void testLoop() { for (int i = 0; i < 1000000; i++) { testLoopOne(a); testLoopOne(snull); } } void testLoopOne(String s) { try { b_setter.invokeExact(this, s); } catch (Throwable t) { throw new Error(t); } } } I am probably obsessing too much over some micro/nano-benchmarks, but i am wondering if this could cause some unwanted de-opt/recompilation effects when all is good with no null values then suddenly a null triggers de-optimization. Paul.

On Jul 9, 2014, at 8:04 PM, John Rose <john.r.rose@oracle.com> wrote:

On Jul 9, 2014, at 3:14 AM, Paul Sandoz <paul.sandoz@oracle.com> wrote:

...

I quickly verified the fold up does as you expect. Also, if i do the following the null check goes away:

...

void testLoop() { for (int i = 0; i < 1000000; i++) { testLoopOne(a); testLoopOne(snull); } }

Good observation. So rather than missing a null-case fold-up (good it's not!), the optimizer is speculating not-nullness (based on profile) and adding a test. (Either the test is being used for a downstream optimization, or else the test is not being detected as useless and removed—which would be bad!.)

I guess it's the pesky null check in the class cast causing issues: public T cast(Object obj) { if (obj != null && !isInstance(obj)) throw new ClassCastException(cannotCastMsg(obj)); return (T) obj; }

...

I am probably obsessing too much over some micro/nano-benchmarks,

(Hi, I'm John and I'm a micro-obsess-aholic.)

:-)

...

but i am wondering if this could cause some unwanted de-opt/recompilation effects when all is good with no null values then suddenly a null triggers de-optimization.

Besides jumping after the micro-benchmark and chewing on the optimizer until the code shrinks, there are two other things we can do:

1. Mentally file the issue and watch real benchmarks for evidence of the problem. (This works pretty well, provided enough time and focus, and provided enough people have some consciousness of the optimizer's workings.)

2. Create a self-test and check it into the test base. It could be either a unit test of assertion. In this case, I don't see an easy way to do it, but creating clever permanent tests almost always pays off much better than cleverly pounding on the micro-benchmark of the moment.

How about the a variant of following? public class NullCheckDroppingsTest { volatile String svalue = "A"; volatile String snull = null; String ssink; public static void main(String[] args) { NullCheckDroppingsTest t = new NullCheckDroppingsTest(); t.testLoop(); } void testLoop() { // Ensure testLoopOne is compiled and does not // see a null value for (int i = 0; i < 1000000; i++) { testLoopOne(svalue); // Uncomment the following and the call outside // the loop should not result in any deoptimization // testLoopOne(snull); } // Sleep just to create delay in the compilation log try { Thread.currentThread().sleep(1000); } catch (Exception e) {} // This should cause a de-optimisation // if testLoopOne is compiled with a null-check testLoopOne(snull); } void testLoopOne(String s) { try { ssink = String.class.cast(s); } catch (Throwable t) { throw new Error(t); } } } $ java -XX:-TieredCompilation -XX:+UnlockDiagnosticVMOptions -XX:+PrintCompilation NullCheckDroppingsTest 64 1 java.lang.String::hashCode (55 bytes) 69 2 java.lang.String::indexOf (70 bytes) 81 3 ! NullCheckDroppingsTest::testLoopOne (27 bytes) 81 5 n java.lang.Class::isInstance (native) 81 4 java.lang.Class::cast (27 bytes) 82 6 % ! NullCheckDroppingsTest::testLoop @ 2 (45 bytes) 85 6 % ! NullCheckDroppingsTest::testLoop @ -2 (45 bytes) made not entrant 1086 3 ! NullCheckDroppingsTest::testLoopOne (27 bytes) made not entrant The log can be searched to see of a deopt ("made not entrant") occurred or not. Paul.

On Jul 8, 2014, at 9:09 PM, John Rose <john.r.rose@oracle.com> wrote:

Regarding the extra cast in accessor logic that Paul picked up on: That may be a left-over from obsolete versions of the code, or it may cover for some corner cases, or it could possibly be a re-assurance to the JIT.

I had some enlightening discussions with Roland on this. It seems quite tricky to solve in general the removal of the null check due to the aggressive nature in which the null branch is reduce to a trap, but IIUC may be possible to turn Class.cast into an intrinsic to handle the specific case (although that seems costly). I was labouring under the misapprehension that an explicit Class.cast was a profiling point but now i realize it's only certain byte codes (like checkcast/invokehandle). Nothing specific to the DHM access logic showed up with regards to type profiling when analysing the MethodData output from some simple examples [*]. Therefore i presume it's more likely to be the first or third reason you state. So i propose to proceed with the experiment with a patch to replace the casts with asserts in the accessor logic and run that through the usual tests. Paul. [*] Also i have so far failed to concoct a simple example for VarHandles where i can trigger profile pollution and failed inlining

On Aug 29, 2014, at 12:45 AM, John Rose <john.r.rose@oracle.com> wrote:

On Aug 28, 2014, at 7:38 AM, Paul Sandoz <paul.sandoz@oracle.com> wrote:

...

On Jul 8, 2014, at 9:09 PM, John Rose <john.r.rose@oracle.com> wrote:

...

Regarding the extra cast in accessor logic that Paul picked up on: That may be a left-over from obsolete versions of the code, or it may cover for some corner cases, or it could possibly be a re-assurance to the JIT.

I had some enlightening discussions with Roland on this.

It seems quite tricky to solve in general the removal of the null check due to the aggressive nature in which the null branch is reduce to a trap, but IIUC may be possible to turn Class.cast into an intrinsic to handle the specific case (although that seems costly).

I was labouring under the misapprehension that an explicit Class.cast was a profiling point but now i realize it's only certain byte codes (like checkcast/invokehandle). Nothing specific to the DHM access logic showed up with regards to type profiling when analysing the MethodData output from some simple examples [*]. Therefore i presume it's more likely to be the first or third reason you state.

So i propose to proceed with the experiment with a patch to replace the casts with asserts in the accessor logic and run that through the usual tests.

Paul.

[*] Also i have so far failed to concoct a simple example for VarHandles where i can trigger profile pollution and failed inlining

Here's something to try first: Force a profile point before Class.cast, even without Roland's enhancements. You should be able to type-profile "x" by inserting "push x; checkcast java/lang/Object; pop". See last line of https://wiki.openjdk.java.net/display/HotSpot/MethodData

Thanks, that's a neat trick. I will play around with that. Some thoughts triggered (now i am a supposedly little wiser... perhaps :-)). I could imagine things would get polluted fairly quickly within the compiled & shared DHM LFs for field access (same for VHs), plus cast-wise only the value is operated on and not the receiver. In general presumably what matters most is the type profile from the call site that would flow down to the LF? But... what would there be in compiled DHM LFs for field access that they would require profiling so that generated code would be different for accessing a field of Bar rather than a field of Foo? since it all boils down to Unsafe calls, or are there some subtle details hidden within the Unsafe intrinsics? or perhaps a lack of that can result in some odd effects? Paul.

Paul, Unfortunately, additional profiling doesn't work for Accessor.checkCast case. The problem is Accessor.checkCast is called from multiple places, so type profile is very likely to be polluted. And it kills the benefits. I don't think MethodType helps with profiling in any way. It represents type info which is necessary for correctness checks. Profiling collects more fine-grained information (e.g. exact types, values). Regarding redundant null check, do you have a test case so I can play with it myself? Best regards, Vladimir Ivanov On 4/1/14 1:55 PM, Paul Sandoz wrote:

On Mar 14, 2014, at 5:36 PM, Vladimir Ivanov <vladimir.x.ivanov@oracle.com> wrote:

...

...

Doh! crossed webrevs, thanks.

Just had a quick look, this looks like a really nice improvement to the array setter/getter support, definitely simplified. IIUC the mh.viewAsType will now handle the appropriate casting. I believe it might reduce the "ceremony" for array setter/getter MHs [1].

I see there is a PROFILE_LEVEL option, by default set to 0, that results in casts not being emitted:

+ if (VerifyType.isNullConversion(Object.class, pclass, false)) { + if (PROFILE_LEVEL > 0) + emitReferenceCast(Object.class, arg); + return; + } ... + mv.visitLdcInsn(constantPlaceholder(cls)); + mv.visitTypeInsn(Opcodes.CHECKCAST, CLS); + mv.visitInsn(Opcodes.SWAP); + mv.visitMethodInsn(Opcodes.INVOKESTATIC, MHI, "castReference", CLL_SIG, false); + if (Object[].class.isAssignableFrom(cls)) + mv.visitTypeInsn(Opcodes.CHECKCAST, OBJARY); + else if (PROFILE_LEVEL > 0) + mv.visitTypeInsn(Opcodes.CHECKCAST, OBJ);

Can you explain a bit the rational for that? These casts are redundant - they aren't required for bytecode correctness. The idea behind PROFILE_LEVEL is to provide more type information to JIT-compiler. Right now, type profiling occurs on every checkcast instruction. So, having these additional instructions we can feed C2 with more accurate information about types.

Consider this as a hack to overcome some of the limitations of current profiling implementation in VM.

Apologies for the late reply this dropped off my radar...

Ah! i may have just had a minor epiphany :-)

So that is why in DirectMethodHandle there are casts for fields, via say Accessor.checkCast?

@Override Object checkCast(Object obj) { return fieldType.cast(obj); }

if so could PROFILE_LEVEL be supported in that code too?

Perhaps the JIT could derive some profile information from the MethodType of the MethodHandle?

I notice that in my experiments for enhanced access to instances of fields that casts are almost optimized away but a null-check is left [*], which is also seems redundant and could impact performance get/set of null values.

Paul.

[*]

0x000000010d050f70: test %r10d,%r10d 0x000000010d050f73: je 0x000000010d050f9d ... 0x000000010d050f9d: mov %rsi,%rbp 0x000000010d050fa0: mov %r10d,0x4(%rsp) 0x000000010d050fa5: mov $0xffffffad,%esi 0x000000010d050faa: nop 0x000000010d050fab: callq 0x000000010d0163e0 ; OopMap{rbp=Oop [4]=NarrowOop off=112} ;*ifnull ; - java.lang.Class::cast@1 (line 3253) ; - java.lang.invoke.InstanceFieldHandle::checkCast@2 (line 133) ; - java.lang.invoke.InstanceFieldHandle::set@19 (line 153) ; - java.lang.invoke.VarHandle::set@21 (line 127) ; - VarHandleTest::testLoopOne@8 (line 157) ; {runtime_call} 0x000000010d050fb0: callq 0x000000010c39d330 ;*ifnull ; - java.lang.Class::cast@1 (line 3253) ; - java.lang.invoke.InstanceFieldHandle::checkCast@2 (line 133) ; - java.lang.invoke.InstanceFieldHandle::set@19 (line 153) ; - java.lang.invoke.VarHandle::set@21 (line 127) ; - VarHandleTest::testLoopOne@8 (line 157) ; {runtime_call}

Best regards, Vladimir Ivanov On 4/1/14 7:29 PM, Paul Sandoz wrote:

On Apr 1, 2014, at 4:10 PM, Vladimir Ivanov <vladimir.x.ivanov@oracle.com> wrote:

...

Paul,

Unfortunately, additional profiling doesn't work for Accessor.checkCast case. The problem is Accessor.checkCast is called from multiple places, so type profile is very likely to be polluted. And it kills the benefits.

So is there any point in doing such a cast in this case?

The type performing the cast is the field type declared as a parameter in the MethodType of the MethodHandle and also held by the Accessor.

IIUC the Invokers.checkExactType should ensure no "unsavoury" instances of the wrong type gets through? (the holder instance is only checked for null, via checkBase).

I don't see any point in doing profiling for this particular case. Such shape should be well optimized by JIT if it sees that an instance of Accessor is a constant. As I understand, it should be the case for most of the usage scenarios. Best regards, Vladimir Ivanov

...

I don't think MethodType helps with profiling in any way. It represents type info which is necessary for correctness checks. Profiling collects more fine-grained information (e.g. exact types, values).

OK.

...

Regarding redundant null check, do you have a test case so I can play with it myself?

I will send something to you later today or tomorrow.

Paul.

On Apr 2, 2014, at 4:16 PM, Paul Sandoz <Paul.Sandoz@oracle.com> wrote:

...

...

...

Regarding redundant null check, do you have a test case so I can play with it myself?

I will send something to you later today or tomorrow.

Still plan to send you something today :-) but later on...

See below for an inline trace, the assembler and the class that was executed with -XX:-TieredCompilation using Java 8. Paul. Inlining _isInstance on constant Class java/lang/String ! @ 9 MHFieldTest::testLoopOne (25 bytes) inline (hot) @ 8 java.lang.invoke.LambdaForm$MH/617901222::invokeExact_MT (15 bytes) inline (hot) @ 2 java.lang.invoke.Invokers::checkExactType (30 bytes) inline (hot) @ 11 java.lang.invoke.MethodHandle::type (5 bytes) accessor @ 11 java.lang.invoke.LambdaForm$MH/523429237::putObjectFieldCast (32 bytes) inline (hot) @ 1 java.lang.invoke.DirectMethodHandle::fieldOffset (9 bytes) inline (hot) @ 6 java.lang.invoke.DirectMethodHandle::checkBase (7 bytes) inline (hot) @ 1 java.lang.Object::getClass (0 bytes) (intrinsic) @ 13 java.lang.invoke.DirectMethodHandle::checkCast (9 bytes) inline (hot) @ 5 java.lang.invoke.DirectMethodHandle$Accessor::checkCast (9 bytes) inline (hot) @ 5 java.lang.Class::cast (27 bytes) inline (hot) @ 6 java.lang.Class::isInstance (0 bytes) (intrinsic) @ 28 sun.misc.Unsafe::putObject (0 bytes) (intrinsic) [Verified Entry Point] 0x000000010ccf0da0: mov %eax,-0x14000(%rsp) 0x000000010ccf0da7: push %rbp 0x000000010ccf0da8: sub $0x20,%rsp ;*synchronization entry ; - MHFieldTest::testLoopOne@-1 (line 57) 0x000000010ccf0dac: mov 0xc(%rsi),%r10d ;*getfield a ; - MHFieldTest::testLoopOne@5 (line 57) 0x000000010ccf0db0: test %r10d,%r10d 0x000000010ccf0db3: je 0x000000010ccf0ddd ;*ifnull ; - java.lang.Class::cast@1 (line 3257) ; - java.lang.invoke.DirectMethodHandle$Accessor::checkCast@5 (line 441) ; - java.lang.invoke.DirectMethodHandle::checkCast@5 (line 510) ; - java.lang.invoke.LambdaForm$MH/640070680::putObjectFieldCast@13 ; - java.lang.invoke.LambdaForm$MH/789451787::invokeExact_MT@11 ; - MHFieldTest::testLoopOne@8 (line 57) 0x000000010ccf0db5: add $0x10,%rsi 0x000000010ccf0db9: mov %r10d,(%rsi) 0x000000010ccf0dbc: mov %rsi,%r10 0x000000010ccf0dbf: shr $0x9,%r10 0x000000010ccf0dc3: mov $0x18f780000,%r11 0x000000010ccf0dcd: mov %r12b,(%r11,%r10,1) ;*getfield a ; - MHFieldTest::testLoopOne@5 (line 57) 0x000000010ccf0dd1: add $0x20,%rsp 0x000000010ccf0dd5: pop %rbp 0x000000010ccf0dd6: test %eax,-0x113ddc(%rip) # 0x000000010cbdd000 ; {poll_return} 0x000000010ccf0ddc: retq 0x000000010ccf0ddd: mov %rsi,%rbp 0x000000010ccf0de0: mov %r10d,0x4(%rsp) 0x000000010ccf0de5: mov $0xffffffad,%esi 0x000000010ccf0dea: nop 0x000000010ccf0deb: callq 0x000000010ccbc120 ; OopMap{rbp=Oop [4]=NarrowOop off=112} ;*ifnull ; - java.lang.Class::cast@1 (line 3257) ; - java.lang.invoke.DirectMethodHandle$Accessor::checkCast@5 (line 441) ; - java.lang.invoke.DirectMethodHandle::checkCast@5 (line 510) ; - java.lang.invoke.LambdaForm$MH/640070680::putObjectFieldCast@13 ; - java.lang.invoke.LambdaForm$MH/789451787::invokeExact_MT@11 ; - MHFieldTest::testLoopOne@8 (line 57) ; {runtime_call} 0x000000010ccf0df0: callq 0x000000010c07ace4 ;*ifnull ; - java.lang.Class::cast@1 (line 3257) ; - java.lang.invoke.DirectMethodHandle$Accessor::checkCast@5 (line 441) ; - java.lang.invoke.DirectMethodHandle::checkCast@5 (line 510) ; - java.lang.invoke.LambdaForm$MH/640070680::putObjectFieldCast@13 ; - java.lang.invoke.LambdaForm$MH/789451787::invokeExact_MT@11 ; - MHFieldTest::testLoopOne@8 (line 57) ; {runtime_call} import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; public class MHFieldTest { volatile String a = "A"; public String b; static final MethodHandle b_setter; static { try { b_setter = MethodHandles.lookup().findSetter(MHFieldTest.class, "b", String.class); } catch (Exception e) { throw new Error(e); } } public static void main(String[] args) { new MHFieldTest().testLoop(); } void testLoop() { for (int i = 0; i < 1000000; i++) { testLoopOne(); } } void testLoopOne() { try { b_setter.invokeExact(this, a); } catch (Throwable t) { throw new Error(t); } } }