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

[Vladimir Ivanov]

John,

What do you think about the following version?
http://cr.openjdk.java.net/~vlivanov/8063137/webrev.02

As you suggested, I reified MHI::profileBranch on LambdaForm level and 
removed @LambdaForm.Shared. My main concern about removing @Sharen was 
that profile pollution can affect the code before profileBranch call 
(akin to 8068915 [1]) and it seems it's the case: Gbemu (at least) is 
sensitive to that change (there's a 10% difference in peak performance 
between @Shared and has_injected_profile()).

I can leave @Shared as is for now or remove it and work on the fix to 
the deoptimization counts pollution. What do you prefer?

Best regards,
Vladimir Ivanov

[1] https://bugs.openjdk.java.net/browse/JDK-8068915

On 1/23/15 4:31 AM, John Rose wrote:
> On Jan 20, 2015, at 11:09 AM, Vladimir Ivanov
> <vladimir.x.ivanov at oracle.com <mailto:vladimir.x.ivanov at oracle.com>> wrote:
>>
>>> 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.
>
> Good.
>
> I think there is a sweeter spot just a little further on.  Make
> profileBranch be an LF intrinsic and expose it like this:
>    GWT(p,t,f;S) := let(a=new int[3]) in lambda(*: S) {
> selectAlternative(profileBranch(p.invoke( *), a), t, f).invoke( *); }
>
> Then selectAlternative triggers branchy bytecodes in the IBGen, and
> profileBranch injects profiling in C2.
> The presence of profileBranch would then trigger the @Shared annotation,
> if you still need it.
>
> After thinking about it some more, I still believe it would be better to
> detect the use of profileBranch during a C2 compile task, and feed that
> to the too_many_traps logic.  I agree it is much easier to stick the
> annotation on in the IBGen; the problem is that because of a minor phase
> ordering problem you are introducing an annotation which flows from the
> JDK to the VM.  Here's one more suggestion at reducing this coupling…
>
> Note that C->set_trap_count is called when each Parse phase processes a
> whole method.  This means that information about the contents of the
> nmethod accumulates during the parse.  Likewise, add a flag method
> C->{has,set}_injected_profile, and set the flag whenever the parser sees
> a profileBranch intrinsic (with or without a constant profile array;
> your call).  Then consult that flag from too_many_traps.  It is true
> that code which is parsed upstream of the very first profileBranch will
> potentially issue a non-trapping fallback, but by definition that code
> would be unrelated to the injected profile, so I don't see a harm in
> that.  If this approach works, then you can remove the annotation
> altogether, which is clearly preferable.  We understand the annotation
> now, but it has the danger of becoming a maintainer's puzzlement.
>
>>
>>> 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.
>
> Good.
>
>>
>>> 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?
>
> That's fine.  Suggest changing the JVM accessor to
> is_lambda_form_shared, because the term "shared" is already overused in
> the VM.
>
> Or, to be much more accurate, s/@Shared/@CollectiveProfile/.  Better
> yet, get rid of it, as suggested above.
>
> (I just realized that profile pollution looks logically parallel to the
> http://en.wikipedia.org/wiki/Tragedy_of_the_commons .)
>
> Also, in the comment explaining the annotation:
>    s/mostly useless/probably polluted by conflicting behavior from
> multiple call sites/
>
> I very much like the fact that profileBranch is the VM intrinsic, not
> selectAlternative.  A VM intrinsic should be nice and narrow like that.
>   In fact, you can delete selectAlternative from vmSymbols while you are
> at it.
>
> (We could do profileInteger and profileClass in a similar way, if that
> turned out to be useful.)
>
> — John