[9] RFR (M): 8063137: Never-taken branches should be pruned when GWT LambdaForms are shared

[Vladimir Ivanov]

John, thanks for the review!

Updated webrev:
http://cr.openjdk.java.net/~vlivanov/8063137/webrev.01/hotspot
http://cr.openjdk.java.net/~vlivanov/8063137/webrev.01/jdk

See my answers inline.

On 1/17/15 2:13 AM, John Rose wrote:
> On Jan 16, 2015, at 9:16 AM, Vladimir Ivanov
> <vladimir.x.ivanov at oracle.com <mailto:vladimir.x.ivanov at oracle.com>> wrote:
>>
>> http://cr.openjdk.java.net/~vlivanov/8063137/webrev.00/hotspot/
>> http://cr.openjdk.java.net/~vlivanov/8063137/webrev.00/jdk/
>> https://bugs.openjdk.java.net/browse/JDK-8063137
>> ...
>> PS: as a summary, my experiments show that fixes for 8063137 & 8068915
>> [2] almost completely recovers peak performance after LambdaForm
>> sharing [3]. There's one more problem left (non-inlined MethodHandle
>> invocations are more expensive when LFs are shared), but it's a story
>> for another day.
>
> This performance bump is excellent news.  LFs are supposed to express
> emergently common behaviors, like hidden classes.  We are much closer to
> that goal now.
>
> I'm glad to see that the library-assisted profiling turns out to be
> relatively clean.
>
> In effect this restores the pre-LF CountingMethodHandle logic from 2011,
> which was so beneficial in JDK 7:
> http://hg.openjdk.java.net/jdk7u/jdk7u/jdk/file/02de5cdbef21/src/share/classes/java/lang/invoke/CountingMethodHandle.java
>
> I have some suggestions to make this version a little cleaner; see below.
>
> Starting with the JDK changes:
>
> In LambdaForm.java, I'm feeling flag pressure from all the little
> boolean fields and constructor parameters.
>
> (Is it time to put in a bit-encoded field "private byte
> LambdaForm.flags", or do we wait for another boolean to come along?  But
> see next questions, which are more important.)
>
> What happens when a GWT LF gets inlined into a larger LF?  Then there
> might be two or more selectAlternative calls.
> Will this confuse anything or will it Just Work?  The combined LF will
> get profiled as usual, and the selectAlternative calls will also collect
> profile (or not?).
>
> This leads to another question:  Why have a boolean 'isGWT' at all?  Why
> not just check for one or more occurrence of selectAlternative, and
> declare that those guys override (some of) the profiling.  Something like:
>
>    -+ if (PROFILE_GWT && lambdaForm.isGWT)
>    ++ if (PROFILE_GWT && lambdaForm.containsFunction(NF_selectAlternative))
> (...where LF.containsFunction(NamedFunction) is a variation of
> LF.contains(Name).)
>
> I suppose the answer may be that you want to inline GWTs (if ever) into
> customized code where the JVM profiling should get maximum benefit.  In
> that case case you might want to set the boolean to "false" to
> distinguish "immature" GWT combinators from customized ones.
>
> If that's the case, perhaps the real boolean flag you want is not
> 'isGWT' but 'sharedProfile' or 'immature' or some such, or (inverting)
> 'customized'.  (I like the feel of a 'customized' flag.)  Then
> @IgnoreProfile would get attached to a LF that (a ) contains
> selectAlternative and (b ) is marked as non-customized/immature/shared.
>   You might also want to adjust the call to 'profileBranch' based on
> whether the containing LF was shared or customized.
>
> What I'm mainly poking at here is that 'isGWT' is not informative about
> the intended use of the flag.
I agree. It was an interim solution. Initially, I planned to introduce 
customization and guide the logic based on that property. But it's not 
there yet and I needed something for GWT case. Unfortunately, I missed 
the case when GWT is edited. In that case, isGWT flag is missed and no 
annotation is set.
So, I removed isGWT flag and introduced a check for selectAlternative 
occurence in LambdaForm shape, as you suggested.

> In 'updateCounters', if the counter overflows, you'll get continuous
> creation of ArithmeticExceptions.  Will that optimize or will it cause a
> permanent slowdown?  Consider a hack like this on the exception path:
>     counters[idx] = Integer.MAX_VALUE / 2;
I had an impression that VM optimizes overflows in Math.exact* 
intrinsics, but it's not the case - it always inserts an uncommon trap. 
I used the workaround you proposed.

> On the Name Bikeshed:  It looks like @IgnoreProfile (ignore_profile in
> the VM) promises too much "ignorance", since it suppresses branch counts
> and traps, but allows type profiles to be consulted.  Maybe something
> positive like "@ManyTraps" or "@SharedMegamorphic"?  (It's just a name,
> and this is just a suggestion.)
What do you think about @LambdaForm.Shared?

> Going to the JVM:
>
> In library_call.cpp, I think you should change the assert to a guard:
>    -+     assert(aobj->length() == 2, "");
>    ++     && aobj->length() == 2) {
Done.

> In Parse::dynamic_branch_prediction, the mere presence of the Opaque4
> node is enough to trigger replacement of profiling.  I think there
> should *not* be a test of method()->ignore_profile().  That should
> provide better integration between the two sources of profile data to
> JVM profiling?
Done.

> Also, I think the name 'Opaque4Node' is way too… opaque.  Suggest
> 'ProfileBranchNode', since that's exactly what it does.
Done.

> Suggest changing the log element "profile_branch" to "observe
> source='profileBranch'", to make a better hint as to the source of the info.
Done.

Best regards,
Vladimir Ivanov