[9] RFR (S): 8157181: Compilers accept modification of final fields outside initializer methods

[Tom Rodriguez]

>>> Therefore, in the current webrev, I've disabled folding of accesses to
>>> all non-stable fields.
>> Please do not do that, it will be a huge regression in term of perf at least for the language runtimes i maintain
>> (and i suppose for Nashorn, JRuby or Groovy), Here are the perf assertions i use routinely,
>> static final method handle is considered as constant, final fields (even not static) of VM anonymous class is considered as truly final.
>> 
> 
> I fully understand your concerns. And thank you for sharing your asserts!

The folding within the compiler should be handled differently since the concerns are different and removing it will likely cause invokeynamic performance to collapse.  The compiler is only folding final instance fields that are part of a chain of constants, so the main concern is whether an object has been published before it’s been fully constructed or if someone is going to use reflection to modify a final field after the object has been published.  The trust_final_non_static_fields logic in ciField.cpp is calling out classes which we control that shouldn’t violate those rules and that are important for performance.  So I think it should be left as is.  It really doesn’t relate to the issue at hand I think.

Also you’ve removed folding of static final fields which would be a very bad thing to do.

ciField has some caching logic for _known_to_link_with_get/set and if you are adding a ciMethod* argument you also need to cache the method that was checked last time as well.

tom

> 
> Let's see what others think about how to go about this problem. So far the following options were explored
> - bail out compilation of non-compliant methods;
> - enforce JVMS conformance for all classes and keep constant folding enabled;
> - enforce JVMS conformance only for classes with _major_version >=52 and completely disable constant folding.
> 
> I'm not sure which option is the best. Also, there might be other options as well.
> 
> Thank you and best regards,
> 
> 
> Zoltan
> 
> 
> 
>>> We could relax that criteria, e.g., by disabling folding only for those
>>> field that were declared in a class with _major_version < 52. But it
>>> would be great if you could give me some feedback before I look into
>>> that. Thank you very much in advance!
>>> 
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~zmajo/8157181/webrev.05/
>>> 
>>> Testing: JPRT in progress.
>>> 
>>> Best regards,
>>> 
>>> 
>>> Zoltan
>> Rémi
>> 
>>>> Thank you and best regards,
>>>> 
>>>> 
>>>> Zoltan
>>>> 
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>> 
>>>>>> I agree with you, alternatively we can propose a more generic fix (fix
>>>>>> the interpreter and the compilers as well). The fix would most likely
>>>>>> affect field resolution in LinkResolver::resolve_field() around here:
>>>>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/interpreter/linkResolver.cpp#l910
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> Changing resolve_field() to throw an IllegalAccessError for
>>>>>> inappropriate field access will propagate the exception to the
>>>>>> interpreter. Changing ciField::will_link() will most likely kill (some)
>>>>>> compilations (if, e.g., fields are linked through
>>>>>> ciField::will_link()).
>>>>>> 
>>>>>> I'd prefer to take the second option (changing field resolution), but
>>>>>> there might be some disadvantages related to option I might be
>>>>>> overseeing.
>>>>> 
>>>>> 
>>>>> 
>>>>>> Thank you!
>>>>>> 
>>>>>> Best regards,
>>>>>> 
>>>>>> 
>>>>>> Zoltan
>>>>>> 
>>>>>> 
>>>>>>>> More details about the failure are here [3].
>>>>>>>> 
>>>>>>>> With the patch applied, the program always completes as it is
>>>>>>>> expected
>>>>>>>> by Jython, no matter if we use -Xint, -Xcomp, or -Xmixed (because we
>>>>>>>> always interpret methods non-conforming with the VM spec).
>>>>>>>> 
>>>>>>>> Here is the updated webrev:
>>>>>>>> http://cr.openjdk.java.net/~zmajo/8157181/webrev.01/
>>>>>>> Bailing out the whole compilation in C2 is even less appropriate. It
>>>>>>> should generate an uncommon trap for such accesses instead (see
>>>>>>> Parse::do_field_access).
>>>>>>> 
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>> 
>>>>>>> [1]
>>>>>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/c1/c1_GraphBuilder.cpp#l1595

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160602/f479359e/attachment.html>