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

Zoltán Majó zoltan.majo at oracle.com
Wed Jun 8 17:55:06 UTC 2016

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;


_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,


> 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

More information about the hotspot-compiler-dev mailing list