[9] [8u40] RFR (M): 8059877: GWT branch frequencies pollution due to LF sharing

Hi Vladimir,

Why do you need getHistoricInt ?
Is it because Unsafe.getInt() doesn't do any constant folding ?

BTW, why getHistoricInt is named getHistoricInt ?

cheers,
R?mi

Post by Vladimir Ivanov
http://cr.openjdk.java.net/~vlivanov/8059877/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8059877
LambdaForm sharing introduces profile pollution in compiled
LambdaForms. The most serious consequence is inlining distortion,
which severely degrade peak performance. The main victim is
guardWithTest (GWT) combinator.
- branch frequencies: never-taken branch is skipped;
- target & fallback method handles (corresponding LFs: compiled vs
interpreted): if method handle has been invoked < COMPILE_THRESHOLD
times, LambdaForm.vmentry points to LF interpreter which is marked w/
@DontInline.
- sharing of GWT LambdaForm pollutes branch profile;
- sharing of LambdaForms used in target & fallback pollutes
invocation counters.
I experimented w/ VM API to guide JIT-compiler using profiling
information gathered on LambdaForm level [1], but decided to take
safer route for now (8u40). JIT-compiler control approach looks
promising, but I need more time to get rid of some performance
artifacts it suffers from.
The proposed fix is to mimic behavior of fully customized LambdaForms.
When GWT is created, both target & fallback method handles are wrapped
reinvoker's compiled LambdaForm). Once a wrapper is invoked more that
DONT_INLINE_THRESHOLD times, it's LambdaForm is replaced with a
regular reinvoker, which is fully transparent for the JIT and it
inlines smoothly.
The downside of the chosen approach is that LambdaForm.updateForm()
doesn't guarantee that all places where stale LambaForm is used see
the update. If it is already part of some nmethod, it won't be
invalidated and recompiled, but will be kept as is. It shouldn't be a
problem, since DONT_INLINE_THRESHOLD is expected to be pretty low
(~30), so only very rarely executed branches are affected.
The fix significantly improves peak performance w/ full LF sharing
(USE_LF_EDITOR=true).
(1) USE_LF_EDITOR=false DONT_INLINE_THRESHOLD=0 (default for 8u40&9)
(2) USE_LF_EDITOR=true DONT_INLINE_THRESHOLD=0 (default for 8u40&9)
(3) USE_LF_EDITOR=true DONT_INLINE_THRESHOLD=30 (fixed version)
(1) & (2) correspond to default configurations (partial & full LF
sharing respectively). (3) is the fixed version.
* Crypto: ~255ms -> ~12ms;
* DeltaBlue: ~40ms -> ~2ms;
* Raytracer: ~62ms -> ~7ms;
* EarleyBoyer: ~160ms -> ~22ms;
* NavierStokes: ~17ms -> ~13ms;
2 subbenchmarks (Box2D & Gbemu) still has some regressions, but it's
Box2D: ~48ms -> ~61ms (w/o the fix: ~155ms)
Gbemu: ~88ms -> ~116ms (w/o the fix: ~160ms)
tests: jck (api/java_lang/invoke), jdk/java/lang/invoke,
jdk/java/util/streams, octane
configurations: -ea -esa -Xverify:all
+ COMPILE_THRESHOLD={0,30} + USE_LF_EDITOR={false,true} +
DONT_INLINE_THRESHOLD={0,30}
Thanks!
Best regards,
Vladimir Ivanov
[1] http://cr.openjdk.java.net/~vlivanov/profiling/
[2] http://cr.openjdk.java.net/~vlivanov/8059877/octane.txt
_______________________________________________
mlvm-dev mailing list
mlvm-dev at openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Vladimir Ivanov

2014-10-10 20:42:27 UTC

Remi,

Post by Remi Forax
Why do you need getHistoricInt ?
Is it because Unsafe.getInt() doesn't do any constant folding ?

Exactly. I need a compile-time constant to feed it to the compiler to
guide compilation.

Post by Remi Forax
BTW, why getHistoricInt is named getHistoricInt ?

From application perspective, the call returns current or some of the
previous values a field has.

Best regards,
Vladimir Ivanov

Post by Remi Forax
cheers,
R?mi

Remi Forax

2014-10-10 21:20:49 UTC

Post by Vladimir Ivanov
Remi,

Post by Remi Forax
Why do you need getHistoricInt ?
Is it because Unsafe.getInt() doesn't do any constant folding ?

Exactly. I need a compile-time constant to feed it to the compiler to
guide compilation.

Post by Remi Forax
BTW, why getHistoricInt is named getHistoricInt ?

From application perspective, the call returns current or some of the
previous values a field has.

thanks for your answer,
I have another question about inOptimizer(),
thinkint a little about it, if there is a code like
if (unsafe.inOptimizer()) {
...
}

this code will always trigger a recompilation, at least once, because in
the interpreter, the branch will never be evaluated and in the JIT,
because inOptimizer will be rue, the JIT will insert a deopt instruction
because the branch was never evaluated before.

I wonder if this recompilation can be avoided or not ?

Post by Vladimir Ivanov
Best regards,
Vladimir Ivanov

regards,
R?mi

Post by Remi Forax
cheers,
R?mi

John Rose

2014-10-11 07:40:10 UTC

Post by Remi Forax
I have another question about inOptimizer(),
thinkint a little about it, if there is a code like
if (unsafe.inOptimizer()) {
...
}
this code will always trigger a recompilation, at least once, because in the interpreter, the branch will never be evaluated and in the JIT,
because inOptimizer will be rue, the JIT will insert a deopt instruction because the branch was never evaluated before.
I wonder if this recompilation can be avoided or not ?

I think so. Being "in the optimizer" is not necessarily the same as being in compiled code. If the JIT were going to deoptimize immediately after an inOptimizer=true condition, then it wasn't really in the optimizer at that point, or (to split hairs) it was at the very edge of partially-optimized code just about to fall out of it.

That's the reason we named it "inOptimizer" instead of "inCompiler". The JIT is free to evaluate it to false, if the path is not well optimized. The overall expectation is that as the code warms up, inOptimizer will go from false to true, eventually.

? John

Remi Forax

2014-10-11 22:11:01 UTC

Post by John Rose

I think so. Being "in the optimizer" is not necessarily the same as being in compiled code. If the JIT were going to deoptimize immediately after an inOptimizer=true condition, then it wasn't really in the optimizer at that point, or (to split hairs) it was at the very edge of partially-optimized code just about to fall out of it.
That's the reason we named it "inOptimizer" instead of "inCompiler". The JIT is free to evaluate it to false, if the path is not well optimized. The overall expectation is that as the code warms up, inOptimizer will go from false to true, eventually.

thanks for the explanation,
my mind was not able to see past the proposed implementation.

Post by John Rose
? John

R?mi

Paul Sandoz

2014-10-13 13:18:08 UTC

Post by Vladimir Ivanov
http://cr.openjdk.java.net/~vlivanov/8059877/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8059877

Generally looks ok.

- MethodHandleImpl

786 if (wrapper.unblock()) {
787 // Reached invocation threshold. Replace counting/blocking wrapper with a reinvoker.
788 wrapper.isActivated = false;

If unblock() returns true then isActivated is already false.

At the expense of another box you could use an AtomicBoolean with compareAndSet if you want to really guarantee at most one unblocking, although the box can be removed if the boolean is changed to 'int' and Unsafe is used to CAS.

I dunno if strengthening the visibility of MethodHandle.form by stamping in a memory fence after the following will help

792 wrapper.updateForm(lform);

e.g. using Unsafe.fullFence.

Perhaps isUnblocked is a better name than isActivated since there is no need to set the initial value and it tracks the same value as that returned from unblock?

Also while on the subject of naming perhaps consider changing MethodTypeForm.LF_DELEGATE_COUNTING to ...LF_DELEGATE_BLOCK_INLINING?

When DONT_INLINE_THRESHOLD > 0 and DONT_INLINE_THRESHOLD == COMPILE_THRESHOLD will that trigger both compilation of a BlockInliningWrapper's form and unblocking? I guess there is some fuzziness depending on concurrent execution.

I am just wondering if there is any point compiling a BlockInliningWrapper's form if DONT_INLINE_THRESHOLD is expected to be ~ the same as COMPILE_THRESHOLD. Probably not terribly important especially if the JIT-compiler control approach replaces it.

Paul.

Vladimir Ivanov

2014-10-14 18:54:52 UTC

Paul,

Thanks for the feedback!

Updated version:
http://cr.openjdk.java.net/~vlivanov/8059877/webrev.01/

Post by Vladimir Ivanov
http://cr.openjdk.java.net/~vlivanov/8059877/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8059877

Fixed.

Post by Paul Sandoz
At the expense of another box you could use an AtomicBoolean with compareAndSet if you want to really guarantee at most one unblocking, although the box can be removed if the boolean is changed to 'int' and Unsafe is used to CAS.

I considered that, but I decided not to go that route. GuardWithTest is
a very common combinator, so footprint overhead could be noticeable.
There's no need to guarantee uniqueness of unblocking operation. The
operation is idempotent, so no problems performing it multiple times.
What I try to achieve with the flag is avoid pathological situation when
some thread continuously updates the form.

Post by Paul Sandoz
I dunno if strengthening the visibility of MethodHandle.form by stamping in a memory fence after the following will help
792 wrapper.updateForm(lform);
e.g. using Unsafe.fullFence.

I think the fence should be there. It won't help guarantee the update is
visible everywhere though. But it is expected.

Post by Paul Sandoz
Perhaps isUnblocked is a better name than isActivated since there is no need to set the initial value and it tracks the same value as that returned from unblock?

Done.

Post by Paul Sandoz
Also while on the subject of naming perhaps consider changing MethodTypeForm.LF_DELEGATE_COUNTING to ...LF_DELEGATE_BLOCK_INLINING?

Done.

Post by Paul Sandoz
When DONT_INLINE_THRESHOLD > 0 and DONT_INLINE_THRESHOLD == COMPILE_THRESHOLD will that trigger both compilation of a BlockInliningWrapper's form and unblocking? I guess there is some fuzziness depending on concurrent execution.

No problems expected in this scenario - COMPILE_THRESHOLD updates
LambdaForm entry point and unblocking operates on a method handle.

Post by Paul Sandoz
I am just wondering if there is any point compiling a BlockInliningWrapper's form if DONT_INLINE_THRESHOLD is expected to be ~ the same as COMPILE_THRESHOLD. Probably not terribly important especially if the JIT-compiler control approach replaces it.

I don't see any value in keeping BlockInliningWrapper interpreted.
It would allow to avoid LambdaForm.forceInline flag, but there should be
a special case to skip compilation (in
LambdaForm::checkInvocationCounter()). Moreover, if we get rid of
LambdaForm interpreter, we will need to precompile BlockInliningWrapper
again.

Best regards,
Vladimir Ivanov

Paul Sandoz

2014-10-15 12:40:30 UTC

Post by Vladimir Ivanov
Paul,
Thanks for the feedback!
http://cr.openjdk.java.net/~vlivanov/8059877/webrev.01/

Post by Vladimir Ivanov
http://cr.openjdk.java.net/~vlivanov/8059877/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8059877

Fixed.

I considered that, but I decided not to go that route. GuardWithTest is a very common combinator, so footprint overhead could be noticeable. There's no need to guarantee uniqueness of unblocking operation. The operation is idempotent, so no problems performing it multiple times. What I try to achieve with the flag is avoid pathological situation when some thread continuously updates the form.

Ok.

I think the fence should be there. It won't help guarantee the update is visible everywhere though. But it is expected.

I see you added it to MethodHandle.updateForm, i was incorrectly assuming you just wanted to add in the context of BlockInliningWrapper after the update call, so not all updates incur the full barrier cost for other calls to updateForm, but it makes sense in light of the ISSUE comment.

Actually i forgot MethodHandle.form is marked final!, i better understand some of your comments now, so visibility is even more restricted that i previously thought.

This is a good example to add to our "stomping on a final field" discussion, i think here it is definitely a very special case with a careful dance between updating and inlining (updateForm is also called by a direct method handle to a static method or field, such a handle initially holds a form with a check to initialize the class and after that occurs the handle is updated with a new form without the check).

Post by Paul Sandoz
Perhaps isUnblocked is a better name than isActivated since there is no need to set the initial value and it tracks the same value as that returned from unblock?

Done.

765 isUnblocked = false;

Should be "isUnblocked = true".

756 MethodHandle newTarget = target.asType(newType);
757 return asTypeCache = isUnblocked ? make(newTarget)
758 : newTarget; // no need for a wrapper anymore

Should be "return asTypeCache = !isUnblocked ? ? make(newTarget)"

No need for another review round.

Post by Paul Sandoz
Also while on the subject of naming perhaps consider changing MethodTypeForm.LF_DELEGATE_COUNTING to ...LF_DELEGATE_BLOCK_INLINING?

Done.

No problems expected in this scenario - COMPILE_THRESHOLD updates LambdaForm entry point and unblocking operates on a method handle.

I don't see any value in keeping BlockInliningWrapper interpreted.
It would allow to avoid LambdaForm.forceInline flag, but there should be a special case to skip compilation (in LambdaForm::checkInvocationCounter()). Moreover, if we get rid of LambdaForm interpreter, we will need to precompile BlockInliningWrapper again.

Ok.

Paul.

Vladimir Ivanov

2014-10-15 15:21:04 UTC

Thanks, Paul!

Post by Vladimir Ivanov
http://cr.openjdk.java.net/~vlivanov/8059877/webrev.01/

...

Post by Paul Sandoz
This is a good example to add to our "stomping on a final field" discussion, i think here it is definitely a very special case with a careful dance between updating and inlining (updateForm is also called by a direct method handle to a static method or field, such a handle initially holds a form with a check to initialize the class and after that occurs the handle is updated with a new form without the check).

Yes, MethodHandle.updateForm is an inherently unsafe mechanism. It
doesn't allow to arbitrarily change method handle behavior at will.
It is used to evolve a method handle to a more performant version when
possible.