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

[Vladimir Ivanov]

> 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()).
Ignore that. Additional runs don't prove there's a regression on Gbemu. 
There's some variance on Gbemu and it's present w/ and w/o @Shared.

Best regards,
Vladimir Ivanov

> 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