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
