JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Martin Buchholz-3
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/
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;
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Martin Buchholz-3
Let's try again without mail bounces ...

On Wed, Jan 3, 2018 at 1:40 PM, Martin Buchholz <[hidden email]> 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/
> 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;
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Martin Buchholz-3
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 <[hidden email]> wrote:

> Let's try again without mail bounces ...
>
>
> On Wed, Jan 3, 2018 at 1:40 PM, Martin Buchholz <[hidden email]>
> 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/
>> 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;
>>
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

John Rose-3
That looks good to me.  Vladimir Ivanov should take a look.
Christmas vacation is just ending in Russia, so he should be around soon.

On Jan 8, 2018, at 6:41 PM, Martin Buchholz <[hidden email]> 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 <[hidden email] <mailto:[hidden email]>> wrote:
> Let's try again without mail bounces ...
>
>
> On Wed, Jan 3, 2018 at 1:40 PM, Martin Buchholz <[hidden email] <mailto:[hidden email]>> 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;
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Paul Sandoz
In reply to this post by Martin Buchholz-3
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 <[hidden email]> 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 <[hidden email]> wrote:
>
>> Let's try again without mail bounces ...
>>
>>
>> On Wed, Jan 3, 2018 at 1:40 PM, Martin Buchholz <[hidden email]>
>> 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/
>>> 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;
>>>
>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Martin Buchholz-3
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();


On Mon, Jan 8, 2018 at 10:50 PM, Paul Sandoz <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
> wrote:
> >
> >> Let's try again without mail bounces ...
> >>
> >>
> >> On Wed, Jan 3, 2018 at 1:40 PM, Martin Buchholz <[hidden email]>
> >> 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/
> >>> 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;
> >>>
> >>>
> >>>
> >>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Vladimir Ivanov
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 <[hidden email]
> <mailto:[hidden email]>> 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 <[hidden email]
>     <mailto:[hidden email]>> 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
>     <[hidden email] <mailto:[hidden email]>> wrote:
>      >
>      >> Let's try again without mail bounces ...
>      >>
>      >>
>      >> On Wed, Jan 3, 2018 at 1:40 PM, Martin Buchholz
>     <[hidden email] <mailto:[hidden email]>>
>      >> 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;
>      >>>
>      >>>
>      >>>
>      >>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Martin Buchholz-3
On Tue, Jan 9, 2018 at 4:12 AM, Vladimir Ivanov <
[hidden email]> wrote:

>
> 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.
>

Thanks for the explanation.

Simple fix was submitted.

I'm still hoping we can later reduce the magicalness of j.l.invoke.

The memory model is already too hard to reason about, but here the VM can
assume that the final fields will never be mutated - yet they are!  With
the special j.l.invoke guarantees, there should be no need for final fields
to additionally have @Stable annotations, yet many do!  LambdaForm is a
mutable class, so publishing it via a plain Unsafe write is a (tiny, hard
to detect) data race.  I would feel much more comfortable replacing the
Unsafe put with a putVolatile and dropping the fence.  Whenever the form
field is read, perhaps it should be explicitly read via a volatile or
acquire read for safety.
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Paul Sandoz


> On 9 Jan 2018, at 14:20, Martin Buchholz <[hidden email]> wrote:
>
> The memory model is already too hard to reason about, but here the VM can assume that the final fields will never be mutated - yet they are!  

Because of reflection and Field/AccessibleObject.setAccessible the VM is conservative and does not in general assume final fields are really final. Because of that we miss out on some juicy optimisations. We have made some inroads into limiting the use of setAccessible. If we can deprecate and remove it we can make progress on generally applying "final means final” to all Java code (which also means tackling the case of deserialisation).


> With the special j.l.invoke guarantees, there should be no need for final fields to additionally have @Stable annotations, yet many do!

In j.l.invoke I only found one redundant @Stable annotated field on MethodType:

  private final @Stable Class<?>   rtype;

All the others are on non-final fields or array fields where the stable semantics are propagated to array elements.


> LambdaForm is a mutable class, so publishing it via a plain Unsafe write is a (tiny, hard to detect) data race.  I would feel much more comfortable replacing the Unsafe put with a putVolatile and dropping the fence.  Whenever the form field is read, perhaps it should be explicitly read via a volatile or acquire read for safety.

That would incur a cost. j.l.invoke contains code that has carefully arranged interactions with the runtime compilers, this is one of those cases.

Paul.
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Martin Buchholz-3
On Tue, Jan 9, 2018 at 2:42 PM, Paul Sandoz <[hidden email]> wrote:

>
>
> > On 9 Jan 2018, at 14:20, Martin Buchholz <[hidden email]> wrote:
> >
> > The memory model is already too hard to reason about, but here the VM
> can assume that the final fields will never be mutated - yet they are!
>
> Because of reflection and Field/AccessibleObject.setAccessible the VM is
> conservative and does not in general assume final fields are really final.
> Because of that we miss out on some juicy optimisations. We have made some
> inroads into limiting the use of setAccessible. If we can deprecate and
> remove it we can make progress on generally applying "final means final” to
> all Java code (which also means tackling the case of deserialisation).
>
>
Will there be a principled way to handle "final means final" for
MethodHandle.form?  Maybe a new annotation that means "mutable BUT compile
as if final" ?


>
> > With the special j.l.invoke guarantees, there should be no need for
> final fields to additionally have @Stable annotations, yet many do!
>
> In j.l.invoke I only found one redundant @Stable annotated field on
> MethodType:
>
>   private final @Stable Class<?>   rtype;
>
> All the others are on non-final fields or array fields where the stable
> semantics are propagated to array elements.
>

Yes, I had forgotten that subtlety with @Stable.


>
> > LambdaForm is a mutable class, so publishing it via a plain Unsafe write
> is a (tiny, hard to detect) data race.  I would feel much more comfortable
> replacing the Unsafe put with a putVolatile and dropping the fence.
> Whenever the form field is read, perhaps it should be explicitly read via a
> volatile or acquire read for safety.
>
> That would incur a cost. j.l.invoke contains code that has carefully
> arranged interactions with the runtime compilers, this is one of those
> cases.
>

(we already have a full fence on writes!)

As it stands, MethodHandle.form is published via a data race which seems
dangerous to me, but may be safe in practice because no one is running Java
on an early Alpha machine.
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Paul Sandoz


> On 9 Jan 2018, at 15:11, Martin Buchholz <[hidden email]> wrote:
>
>
>
> On Tue, Jan 9, 2018 at 2:42 PM, Paul Sandoz <[hidden email] <mailto:[hidden email]>> wrote:
>
>
> > On 9 Jan 2018, at 14:20, Martin Buchholz <[hidden email] <mailto:[hidden email]>> wrote:
> >
> > The memory model is already too hard to reason about, but here the VM can assume that the final fields will never be mutated - yet they are!
>
> Because of reflection and Field/AccessibleObject.setAccessible the VM is conservative and does not in general assume final fields are really final. Because of that we miss out on some juicy optimisations. We have made some inroads into limiting the use of setAccessible. If we can deprecate and remove it we can make progress on generally applying "final means final” to all Java code (which also means tackling the case of deserialisation).
>
>
> Will there be a principled way to handle "final means final" for MethodHandle.form?  Maybe a new annotation that means "mutable BUT compile as if final" ?
>  

Yes, i could imagine in the future removing the final modifier and adding the @Stable annotation or some variant of for the more “relaxed" form of finality that is currently supported. In fact it might be clearer if we do that now (make non-final and add @Stable).


>
> > With the special j.l.invoke guarantees, there should be no need for final fields to additionally have @Stable annotations, yet many do!
>
> In j.l.invoke I only found one redundant @Stable annotated field on MethodType:
>
>   private final @Stable Class<?>   rtype;
>
> All the others are on non-final fields or array fields where the stable semantics are propagated to array elements.
>
> Yes, I had forgotten that subtlety with @Stable.
>  
>
> > LambdaForm is a mutable class, so publishing it via a plain Unsafe write is a (tiny, hard to detect) data race.  I would feel much more comfortable replacing the Unsafe put with a putVolatile and dropping the fence.  Whenever the form field is read, perhaps it should be explicitly read via a volatile or acquire read for safety.
>
> That would incur a cost. j.l.invoke contains code that has carefully arranged interactions with the runtime compilers, this is one of those cases.
>
> (we already have a full fence on writes!)
>

True, but the writes should be rare compared to the reads and as Vladimir points out the customisation should not change the functionality.

Paul.

> As it stands, MethodHandle.form is published via a data race which seems dangerous to me, but may be safe in practice because no one is running Java on an early Alpha machine.

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Martin Buchholz-3
On Tue, Jan 9, 2018 at 4:50 PM, Paul Sandoz <[hidden email]> wrote:

>
>
> On 9 Jan 2018, at 15:11, Martin Buchholz <[hidden email]> wrote:
>
>
>
> On Tue, Jan 9, 2018 at 2:42 PM, Paul Sandoz <[hidden email]>
> wrote:
>
>>
>>
>> > On 9 Jan 2018, at 14:20, Martin Buchholz <[hidden email]> wrote:
>> >
>> > The memory model is already too hard to reason about, but here the VM
>> can assume that the final fields will never be mutated - yet they are!
>>
>> Because of reflection and Field/AccessibleObject.setAccessible the VM is
>> conservative and does not in general assume final fields are really final.
>> Because of that we miss out on some juicy optimisations. We have made some
>> inroads into limiting the use of setAccessible. If we can deprecate and
>> remove it we can make progress on generally applying "final means final” to
>> all Java code (which also means tackling the case of deserialisation).
>>
>>
> Will there be a principled way to handle "final means final" for
> MethodHandle.form?  Maybe a new annotation that means "mutable BUT compile
> as if final" ?
>
>
>
> Yes, i could imagine in the future removing the final modifier and adding
> the @Stable annotation or some variant of for the more “relaxed" form of
> finality that is currently supported. In fact it might be clearer if we do
> that now (make non-final and add @Stable).
>
>
Well, using @Stable here would be very much undefined

 * It is (currently) undefined what happens if a field annotated as stable
 * is given a third value (by explicitly updating a stable field, a
component of
 * a stable array, or a final stable field via reflection or other means).
 * Since the HotSpot VM promotes a non-null component value to constant, it
may
 * be that the Java memory model would appear to be broken, if such a
constant
 * (the second value of the field) is used as the value of the field even
after
 * the field value has changed (to a third value).


>> > LambdaForm is a mutable class, so publishing it via a plain Unsafe
>> write is a (tiny, hard to detect) data race.  I would feel much more
>> comfortable replacing the Unsafe put with a putVolatile and dropping the
>> fence.  Whenever the form field is read, perhaps it should be explicitly
>> read via a volatile or acquire read for safety.
>>
>> That would incur a cost. j.l.invoke contains code that has carefully
>> arranged interactions with the runtime compilers, this is one of those
>> cases.
>>
>
> (we already have a full fence on writes!)
>
>
> True, but the writes should be rare compared to the reads and as Vladimir
> points out the customisation should not change the functionality.
>

We're changing a "truly" final field holding a mutable object via a data
race.  Subsequently we may randomly mix the cached old object fields and
the new object, and perhaps even uninitialized fields of the latter.  In
theory.


> Paul.
>
> As it stands, MethodHandle.form is published via a data race which seems
> dangerous to me, but may be safe in practice because no one is running Java
> on an early Alpha machine.
>
>
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Paul Sandoz


> On 9 Jan 2018, at 23:28, Martin Buchholz <[hidden email]> wrote:
>
>
>
> On Tue, Jan 9, 2018 at 4:50 PM, Paul Sandoz <[hidden email] <mailto:[hidden email]>> wrote:
>
>
>> On 9 Jan 2018, at 15:11, Martin Buchholz <[hidden email] <mailto:[hidden email]>> wrote:
>>
>>
>>
>> On Tue, Jan 9, 2018 at 2:42 PM, Paul Sandoz <[hidden email] <mailto:[hidden email]>> wrote:
>>
>>
>> > On 9 Jan 2018, at 14:20, Martin Buchholz <[hidden email] <mailto:[hidden email]>> wrote:
>> >
>> > The memory model is already too hard to reason about, but here the VM can assume that the final fields will never be mutated - yet they are!
>>
>> Because of reflection and Field/AccessibleObject.setAccessible the VM is conservative and does not in general assume final fields are really final. Because of that we miss out on some juicy optimisations. We have made some inroads into limiting the use of setAccessible. If we can deprecate and remove it we can make progress on generally applying "final means final” to all Java code (which also means tackling the case of deserialisation).
>>
>>
>> Will there be a principled way to handle "final means final" for MethodHandle.form?  Maybe a new annotation that means "mutable BUT compile as if final" ?
>>  
>
> Yes, i could imagine in the future removing the final modifier and adding the @Stable annotation or some variant of for the more “relaxed" form of finality that is currently supported. In fact it might be clearer if we do that now (make non-final and add @Stable).
>
>
> Well, using @Stable here would be very much undefined
>
>  * It is (currently) undefined what happens if a field annotated as stable
>  * is given a third value (by explicitly updating a stable field, a component of
>  * a stable array, or a final stable field via reflection or other means).
>  * Since the HotSpot VM promotes a non-null component value to constant, it may
>  * be that the Java memory model would appear to be broken, if such a constant
>  * (the second value of the field) is used as the value of the field even after
>  * the field value has changed (to a third value).

Yes :-) In this case the form field is updated with the knowledge that visibility but not functionality may be affected.

Note that final fields of classes in java.lang.invoke are in effect implicitly annotated with @Stable (that’s why i referred to the notion of “relaxed” finality).


>
>>
>> > LambdaForm is a mutable class, so publishing it via a plain Unsafe write is a (tiny, hard to detect) data race.  I would feel much more comfortable replacing the Unsafe put with a putVolatile and dropping the fence.  Whenever the form field is read, perhaps it should be explicitly read via a volatile or acquire read for safety.
>>
>> That would incur a cost. j.l.invoke contains code that has carefully arranged interactions with the runtime compilers, this is one of those cases.
>>
>> (we already have a full fence on writes!)
>>
>
> True, but the writes should be rare compared to the reads and as Vladimir points out the customisation should not change the functionality.
>
> We're changing a "truly" final field holding a mutable object via a data race.  Subsequently we may randomly mix the cached old object fields and the new object, and perhaps even uninitialized fields of the latter.  In theory.
>  

Given how the compiler consumes values of these fields i am unsure how that can happen.

The code is skating close to the edge and is definitely making you uncomfortable :-) so I proposed making the field non-final but annotated with @Stable, the same optimisations should still apply.

Paul.
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

Martin Buchholz-3
On Wed, Jan 10, 2018 at 3:58 PM, Paul Sandoz <[hidden email]> wrote:

>
> The code is skating close to the edge and is definitely making you
> uncomfortable :-) so I proposed making the field non-final but annotated
> with @Stable, the same optimisations should still apply.
>

Sorry, @Stable doesn't increase my comfort.  It is common for unsafe
publication bugs to persist for years because they don't manifest as
user-visible behavior very often.  Like
 https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html