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

Fri Jun 3 10:48:38 UTC 2016

On 6/3/16 7:11 AM, Vladimir Kozlov wrote:
> How about only fixing C1 to take into account the store (or not constant
> fold in such case) because it was allowed before JVMS-7?
>
> And file a separate issue for JVMS-7 conformance. It looks like it may
> have huge impact on performance and we are already very late in JDK 9
> development to make such changes in rush.

Yes, let's separate them (considering the difference in impact) and 
align the JVM behavior with the JVMS separately.

For now, disabling constant folding of static/instance field loads in 
static/instance initializers should solve the immediate problem for 
well-behaved (according to JVMS) programs. Also, such change should not 
cause any performance regressions.

Considering it is possible to update static final fields through 
Reflection API, I don't see JVMS conformance issue (ability to update 
final static fields outside of static initializer on bytecode level) as 
a problem of the same level for JIT-compilers. It can be fixed as part 
of aligning the JVM with JVMS (but still in JDK9).

Best regards,
Vladimir Ivanov

>
> Zoltan, what C2 does in such case?
> Tom, what Graal does in such case?
>
> The case is:
>
> https://bugs.openjdk.java.net/browse/JDK-8157181?focusedCommentId=13942824&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13942824
>
>
> "While compiling bytecode @29, the C1 compiler assumes that the value of
> the _pyx0.self field has already been set in the class initializer
> <clinit> method (as the class is initialized). The C1 compiler also
> assumes that the field will not change (as it is static final).
> Therefore, the C1 compiler omits reading the field _pyx0.self and passes
> the current value of the field ('null') to the
> org/python/core/Py.newCode() method called at bytecode @37. (The correct
> value would be 'this'.)"
>
> Thanks,
> Vladimir
>
> On 6/2/16 10:51 AM, Tom Rodriguez wrote:
>>>>> 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
>>>>>>>>>
>>