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

Zoltán Majó zoltan.majo at oracle.com
Wed Jun 8 18:00:10 UTC 2016 

Hi Vladimir,


thank you for the feedback!

On 06/03/2016 06: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?

before JVMS-7, any method of a class 'C' (not just the class 
initializer) is allowed to modify a *static* final field 'C::f' of that 
class.  The same is true for *instance* final fields (any method, not 
only constructors, is allowed to modify them).

So, to avoid problems similar to the current one, we would have to 
disable constant folding completely if JVMS-7 is not enforced by the VM. 
That is not a walkable path due to performance reasons.

Fortunately, there are *very few* classes that "misbehave" the above 
way. (So far I've seen *only two* such classes, Jython and a single JCK 
test, although I've run all pre-PIT testing with -Xcomp and -Xmixed.)

So, a solution would be to detect misbehaving fields and disable 
constant folding only for those fields. And that is what the latest 
webrev (webrev.06) does.

To summarize, webrev.06
- *detects misbehaving fields in the verifier* for classes with version 
< JAVA_7_VERSION and *disables constant folding for those fields only*;
- *enforces JVMS-7 conformance* for classes with version >= 
JAVA_7_VERSION and *leaves constant folding as it is* (no performance 
impact).

As I mentioned to David Holmes, we can use JAVA_8_VERSION or 
JAVA_9_VERSION instead of JAVA_7_VERSION.

I'm currently running pre-PIT RBT to see if there are any problems with 
our testbase. Performance runs are also in progress.

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

More information about the hotspot-compiler-dev mailing list