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.iva...@oracle.com <mailto:vladimir.x.iva...@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
