JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Tue Jan 9 12:12:57 UTC 2018
Martin,
The fix (MH.customize() part) looks good.
Thanks for taking care of it!
If there's a concurrent customization taking place, repeated read of
MH.form can observe customized LF and repeat customization thus leading
to a customized LF pointing to another customized LF (by
LF.transformCache). Then LambdaFormEditor expects uncustomized LF (in
factory method LFE.lambdaFormEditor(LF)) and calls LF.uncustomize(), but
the latter doesn't expect nested customized LFs and just returns the
immediate value from LF.transformCache which is a customized LF. And
that's why probing LF cache fails later with a CCE.
So, caching MH.form value and redoing LF customization (in the worst
case) is safe and fixes the bug.
It would be nice to have a regression test (or an assert), but I'm fine
with it as is.
> In the hotspot sources I find
> // An instance field can be constant if it's a final static field
> or if
> // it's a final non-static field of a trusted class (classes in
> // java.lang.invoke and sun.invoke packages and subpackages).
> BUT
> you can't trust MethodHandle.form!
> Yes, it's
> final LambdaForm form;
> but it's also frobbed via Unsafe, and in an unusual unsafe way:
> UNSAFE.putObject(this, FORM_OFFSET, newForm);
> UNSAFE.fullFence();
Constant folding of MH.form is crucial for performance: without it no
inlining through MethodHandle chains would happen.
It is expected that MH.form updates aren't guaranteed to be visible (and
it's not a theoretical possibility, but a pretty common scenario in
JIT-compiled code (both by C1 & C2)). So, observing old MH.form values
should be benign.
Here's the relevant part of MH.updateForm() contract:
/**
* Replace the old lambda form of this method handle with a new one.
* The new one must be functionally equivalent to the old one.
...
void updateForm(LambdaForm newForm) {
It is used to improve performance (remove class init check, customize LF
for a particular MH instance) and not to arbitrarily change behavior of
existing MH instance.
Actual code on writer side (in MH.updateForm) doesn't matter much (it
can be a volatile or even a plain field write) while readers treat the
field as a plain final.
There's an option to invalidate all compiled code where old MH.form
value is used (akin to how CallSite.target is optimized), but it doesn't
worth it most of the time: invalidating C1-compiled code isn't needed in
tiered mode (it'll be recompiled by C2 anyway), recompilation is
expensive (waste of resources), and possible performance improvements
are limited.
So, having a full fence there (to improve how prompt update is visible
to readers w/o affecting them) is as good as other options IMO and I'd
leave it as is for now.
Best regards,
Vladimir Ivanov
> On Mon, Jan 8, 2018 at 10:50 PM, Paul Sandoz <paul.sandoz at oracle.com
> <mailto:paul.sandoz at oracle.com>> wrote:
>
> Hi Martin
>
> [Back from holiday]
>
> Can you reproduce using the debugger? If so do you have a more up to
> date stack trace?
>
> That change looks ok. The form field is final, and in the j.l.invoke
> package such fields are also stable so once C2 gets at it it will
> likely treat it as a constant. In general the updates to the field
> will only be visible to interpreted code or recompiled code.
>
> I suspect the problematic field may well be that of
> LambdaForm.transformCache.
>
> Paul.
>
> > On 8 Jan 2018, at 18:41, Martin Buchholz <martinrb at google.com
> <mailto:martinrb at google.com>> wrote:
> >
> > No takers? ... let's try again. Despite uniform failure to
> reproduce this
> > except under a debugger, let's apply the simple obviously correct
> fix,
> > namely:
> >
> > /** Craft a LambdaForm customized for this particular
> MethodHandle */
> > /*non-public*/
> > void customize() {
> > + final LambdaForm form = this.form;
> > if (form.customized == null) {
> > LambdaForm newForm = form.customize(this);
> > updateForm(newForm);
> >
> >
> >
> >
> > On Wed, Jan 3, 2018 at 1:51 PM, Martin Buchholz
> <martinrb at google.com <mailto:martinrb at google.com>> wrote:
> >
> >> Let's try again without mail bounces ...
> >>
> >>
> >> On Wed, Jan 3, 2018 at 1:40 PM, Martin Buchholz
> <martinrb at google.com <mailto:martinrb at google.com>>
> >> wrote:
> >>
> >>> Here at Google we tried to fix JDK-8145371 because it looked
> like it was
> >>> causing rare problems in production. But after looking at the
> code, I'm
> >>> not sure... Anyways,
> >>>
> >>>
> http://cr.openjdk.java.net/~martin/webrevs/jdk/MethodHandle.form/
> <http://cr.openjdk.java.net/~martin/webrevs/jdk/MethodHandle.form/>
> >>> https://bugs.openjdk.java.net/browse/JDK-8145371
> <https://bugs.openjdk.java.net/browse/JDK-8145371>
> >>> Copying to a local in customize() must be an improvement.
> >>> The UNSAFE publication in updateForm looks fishy, as does that
> comment
> >>> in MethodHandleImpl.java. Is there something the fullFence()
> is supposed
> >>> to do that isn't taken care of by putObjectVolatile ?
> >>>
> >>> Feel free to take ownership of this bug from me.
> >>>
> >>> ---
> a/src/java.base/share/classes/java/lang/invoke/MethodHandle.java
> >>> +++
> b/src/java.base/share/classes/java/lang/invoke/MethodHandle.java
> >>> @@ -1660,13 +1660,13 @@
> >>> assert(newForm.customized == null || newForm.customized
> == this);
> >>> if (form == newForm) return;
> >>> newForm.prepare(); // as in MethodHandle.<init>
> >>> - UNSAFE.putObject(this, FORM_OFFSET, newForm);
> >>> - UNSAFE.fullFence();
> >>> + UNSAFE.putObjectVolatile(this, FORM_OFFSET, newForm);
> >>> }
> >>>
> >>> /** Craft a LambdaForm customized for this particular
> MethodHandle */
> >>> /*non-public*/
> >>> void customize() {
> >>> + final LambdaForm form = this.form;
> >>> if (form.customized == null) {
> >>> LambdaForm newForm = form.customize(this);
> >>> updateForm(newForm);
> >>> diff --git
> a/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java
> b/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java
> >>> ---
> a/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java
> >>> +++
> b/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java
> >>> @@ -909,7 +909,7 @@
> >>> int c = count;
> >>> maybeCustomizeTarget();
> >>> if (c <= 1) {
> >>> - // Try to limit number of updates.
> MethodHandle.updateForm() doesn't guarantee LF update visibility.
> >>> + // Try to limit number of updates.
> >>> if (isCounting) {
> >>> isCounting = false;
> >>> return true;
> >>>
> >>>
> >>>
> >>
>
>
More information about the core-libs-dev
mailing list