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

[Zoltán Majó]

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