JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

[Paul Sandoz]

> On 9 Jan 2018, at 23:28, Martin Buchholz <martinrb at google.com> wrote:
> 
> 
> 
> On Tue, Jan 9, 2018 at 4:50 PM, Paul Sandoz <paul.sandoz at oracle.com <mailto:paul.sandoz at oracle.com>> wrote:
> 
> 
>> On 9 Jan 2018, at 15:11, Martin Buchholz <martinrb at google.com <mailto:martinrb at google.com>> wrote:
>> 
>> 
>> 
>> On Tue, Jan 9, 2018 at 2:42 PM, Paul Sandoz <paul.sandoz at oracle.com <mailto:paul.sandoz at oracle.com>> wrote:
>> 
>> 
>> > On 9 Jan 2018, at 14:20, Martin Buchholz <martinrb at google.com <mailto:martinrb at google.com>> 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.