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

Fri Jun 3 10:59:11 UTC 2016

(forgot to add hotspot-compiler-dev@)

Best regards,
Vladimir Ivanov

On 6/3/16 1:48 PM, Vladimir Ivanov wrote:
>
>
> 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
>>>>>>>>>>
>>>>>>>>>>
>>>