[9] RFR (X): 8163880: Constant pool caching of fields inhibited/delayed unnecessarily

Zoltán Majó zoltan.majo at oracle.com
Mon Aug 29 05:28:28 UTC 2016 

Thank you, Vladimir, for the review!

Best regards,


Zoltan


On 08/26/2016 07:58 PM, Vladimir Kozlov wrote:
> Looks good.
>
> Thanks,
> Vladimir
>
> On 8/26/16 4:38 AM, Zoltán Majó wrote:
>> Hi Vladimir,
>>
>>
>> thank you for looking at this issue once more.
>>
>> On 08/25/2016 09:05 PM, Vladimir Kozlov wrote:
>>> Okay I think I understand what you are trying to say but it was not my
>>> point.
>>
>> Oh, I see.  Then I misunderstood you.
>>
>>> I was questioning why put_code is set when bytecode could be get
>>> instruction. Reading comments and code around I think I understand now.
>>> We set both get_code and put_code marking that the field is resolved.
>>> But in some cases we delay marking to force field's reresolution.
>>
>> Yes, that is also my understanding.
>>
>>>
>>> In next code you can use is_static:
>>>
>>>     bool uninitialized_static = ((bytecode == Bytecodes::_getstatic ||
>>> bytecode == Bytecodes::_putstatic) &&
>>>                                  !klass->is_initialized());
>>
>> OK, I've updated the code.
>>
>>>
>>> Code could be rearranged to be more compact (similar to original code):
>>>
>>>     Bytecodes::Code put_code = (Bytecodes::Code)0;
>>>     Bytecodes::Code get_code = (Bytecodes::Code)0;
>>>     if (!uninitialized_static) {
>>>       get_code = ((is_static) ? Bytecodes::_getstatic :
>>> Bytecodes::_getfield);
>>>       if ((is_put && !has_initialized_final_update) ||
>>> !info.access_flags().is_final()) {
>>>         put_code = ((is_static) ? Bytecodes::_putstatic :
>>> Bytecodes::_putfield);
>>>       }
>>>     }
>>
>> I agree and have updated the code accordingly.
>>
>> Here is the updated webrev:
>> http://cr.openjdk.java.net/~zmajo/8163880/webrev.02/
>>
>> JPRT testing passes (testset hotspot).
>>
>> Thank you!
>>
>> Best regards,
>>
>>
>> Zoltan
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 8/23/16 5:31 AM, Zoltán Majó wrote:
>>>> Hi Vladimir,
>>>>
>>>>
>>>> thank you for the feedback! Please see details below.
>>>>
>>>> On 08/22/2016 08:20 PM, Vladimir Kozlov wrote:
>>>>> Typo (double 'not'):
>>>>> +   // and the required IllegalAccessError would not not be thrown.
>>>>
>>>> OK, I've corrected that.
>>>>
>>>>>
>>>>> I think has_initialized_final_update should be part of assert to see
>>>>> it in hs_err:
>>>>>
>>>>> +   if (has_initialized_final_update) {
>>>>> +     assert(info.access_flags().is_final(), "Fields with initialized
>>>>> final updates must be final");
>>>>> +   }
>>>>
>>>> Yes, I've updated that as well.
>>>>
>>>>>
>>>>> Why is_put check is not done in outer check? It is missing for
>>>>> is_final():
>>>>>
>>>>> +   if (!uninitialized_static) {
>>>>> +     if ((is_put && !has_initialized_final_update) ||
>>>>> !info.access_flags().is_final()) {
>>>>
>>>> I think it's an optimization to do it this way: For 
>>>> non-final-fields, we
>>>> want to set up CP caching faster (i.e., we want to reduce the 
>>>> number of
>>>> times we call into the runtime to
>>>> LinkResolver::resolve_field_access()).We had that optimization in 
>>>> place
>>>> before JDK-8160527, but not afterwards, which caused a performance
>>>> regression.  Please see some details below.
>>>>
>>>> The CP cache does caching for get and put bytecodes individually.
>>>> Normally, the caching for put (in 'put_code') should be set up by put
>>>> instructions, the caching for get (in 'get_code') should be set up by
>>>> get instruction.  The optimization is to let get instructions set up
>>>> caching for put bytecodes in some select cases.  Here is the way that
>>>> (not) workedbefore/after JDK-8160527.
>>>>
>>>>
>>>> 1) The table below describes the way resolution worked before
>>>> JDK-8160527.  The table shows if put_code was set (S) or not set (NS)
>>>> after InterpreterRuntime::resolve_get_put().
>>>>
>>>>                | Instruction triggering resolution
>>>>                |===================================
>>>> Field type     | get          | put
>>>> ===================================================
>>>> final          | NS           |S
>>>> non-final      | S            | S
>>>>
>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/29ed49c42486#l1.38
>>>>
>>>>
>>>> 2) Unfortunately, JDK-8160527 changed the table to:
>>>>
>>>>                | Instruction triggering resolution
>>>>                |===================================
>>>> Field type     | get          | put
>>>> ===================================================
>>>> final          | NS           |*NS*
>>>> non-final      | *NS*         | S
>>>>
>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/29ed49c42486#l1.30
>>>>
>>>> So the VM does not set up caching in *2additional cases*, which 
>>>> causes a
>>>> performance regression.
>>>>
>>>>
>>>> 3) The current webrev proposes the following:
>>>>
>>>>                | Instruction triggering resolution
>>>>                |===================================
>>>> Field type     | get          | put
>>>> ===================================================
>>>> final          | NS           |*S (unless has_initialized_final_update
>>>> is true for the field)*
>>>> non-final      | S            | S
>>>>
>>>> That table looks the same as what we had before JDK-8160527, except 
>>>> for
>>>> fields with initialized final updates. For those fields we need to 
>>>> defer
>>>> setting up CP caching so that the appropriate exception can be 
>>>> thrown at
>>>> the right place (by LinkResolver::resolve_field_access()).
>>>>
>>>>
>>>> 4) If we would do what you suggested (I hope I understood your
>>>> suggestion correctly)thenwe'd move the is_put condition to the outer
>>>> check:
>>>>
>>>> +   if (!uninitialized_static && is_put) {
>>>> +     if (!has_initialized_final_update ||
>>>> !info.access_flags().is_final()) {
>>>>
>>>> Then the table would look:
>>>>
>>>>                | Instruction triggering resolution
>>>>                |===================================
>>>> Field type     | get          | put
>>>> ===================================================
>>>> final          | NS           |S(unless 
>>>> has_initialized_final_update is
>>>> true for the field)
>>>> non-final      | *NS*         | S
>>>>
>>>> So we would have *still one case* where CP caching is not set up and
>>>> that would most likely result in some performance regression (relative
>>>> to pre-JDK-8160527).
>>>>
>>>>
>>>> I hope I have not overlooked anythingabove.
>>>>
>>>> Here is the updated webrev:
>>>> http://cr.openjdk.java.net/~zmajo/8163880/webrev.01/
>>>>
>>>> JPRT testing is running.
>>>>
>>>> Thank you!
>>>>
>>>> Best regards,
>>>>
>>>>
>>>> Zoltan
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 8/22/16 4:20 AM, Zoltán Majó wrote:
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> please review the fix for JDK-8163880.
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8163880
>>>>>>
>>>>>> Problem & solution: The performance regression reported by the 
>>>>>> bug is
>>>>>> caused by unnecessary delay (or even inhibition) of constant pool
>>>>>> caching for fields targeted by put instructions. For a more
>>>>>> detailed description of the problem and possible solutions, please
>>>>>> see the following comment in the bug report [1].
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~zmajo/8163880/webrev.00/
>>>>>>
>>>>>> Testing:
>>>>>> - performance evaluation with the affected benchmark shows that fix
>>>>>> above solves the performance regression;
>>>>>> - all hotspot test with -Xmixed and -Xcomp (running).
>>>>>>
>>>>>> Thank you!
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>>
>>>>>> Zoltan
>>>>>>
>>>>>> [1]
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8163880?focusedCommentId=13990030&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13990030 
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>

More information about the hotspot-dev mailing list