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

[Zoltán Majó]

Hi Tom,


thank you for your input and suggestions! Please see comments inline.

On 06/02/2016 07:51 PM, 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.

Thank you for clarifying -- I agree and won't touch the folding of final 
instance fields.

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

Yes, you're right. But I was curious of how much degradation does the 
removal of folding (most) static final fields cause. So, *as a side 
experiment*, I have changed the logic controlling folding of static 
final fields [1] from

_is_constant = true;

to

_is_constant = is_stable_field || trust_final_non_static_fields(_holder);

(that is similar to what we do for instance final fields).

SPECjvm2008 is unaffected. For the Octane benchmarks we get <10% 
degradation in 66/70 different configurations, <20% degradation in the 
remaining cases.

The experiment was done on the side and related changes are not included 
in the newest webrev (webrev.06).

>
> 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.

Thanks for pointing that out! I've addressed the caching of methods in 
the webrev.06 (URL in my reply to Vladimir K.)

Best regards,


Zoltan

[1]: 
http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/84ff58dfd5e0/src/share/vm/ci/ciField.cpp#l256
>
> 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/ 
>>>> <http://cr.openjdk.java.net/%7Ezmajo/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/ 
>>>>>>>>> <http://cr.openjdk.java.net/%7Ezmajo/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
>