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

[Zoltán Majó]

Hi,


On 06/08/2016 08:00 PM, Zoltán Majó wrote:
> [...]
> I'm currently running pre-PIT RBT to see if there are any problems 
> with our testbase. Performance runs are also in progress.

Just to report on the performance/correctness results: The change does 
not cause any performance regression; pre-PIT RBT completed successfully.

Thank you!

Best regards,


Zoltan

>
> Here is the updated webrev:
> http://cr.openjdk.java.net/~zmajo/8157181/webrev.06/
>
> Testing:
> - JPRT;
> - local testing with reproducer (verified that interpreter/compiled 
> code behave the same way);
> - pre-PIT RBT in progress.
>
>
>>
>> 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.
>
> I agree that the two issues can be treated as unrelated. But, for now, 
> I'd like to keep them together as by fixing them together we cover 
> both the > JAVA_7_VERSION and the <= JAVA_7_VERSION case at the same 
> time.
>
> The performance runs (in progress) show that the fix has no 
> performance impact (because we disable constant folding in very rare 
> cases). I'll report all numbers once they become available.
>
>>
>> Zoltan, what C2 does in such case?
>
> I can reproduce the problem only with -Xcomp. With -Xcomp, there is 
> not enough profiling information available for C2 to compile the 
> problematic bytecode. Thus, C2 deoptimizes before execution proceeds 
> to that bytecode.
>
> But C2 folds final fields as well, therefore C2-compiled methods can 
> also be negatively impacted if final fields chang after they have been 
> initialized. Fortunately, with the current fix we have to disable 
> folding only in very rare cases.
>
> Thank you!
>
> Best regards,
>
>
> Zoltan
>
>> 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 
>>>>>>>>>>
>>>
>