RFR: 8187033: [PPC] Imporve performance of ObjectStreamClass.getClassDataLayout()

[Kazunori Ogata]

Hello,

Could someone review this patch?

I think this patch works correctly.  Although making 
ObjectStreamClass.dataLayout non-volatile can cause benign race, it still 
works correctly.  Even when two or more threads competes to store 
references to the field, the ClassDataSlots[] objects are equivalent 
because it represents the layout of the same class.


Regards,
Ogata



From:   Kazunori Ogata/Japan/IBM
To:     Peter Levart <peter.levart at gmail.com>
Cc:     core-libs-dev <core-libs-dev at openjdk.java.net>, Hans Boehm 
<hboehm at google.com>, Aleksey Shipilev <shade at redhat.com>
Date:   2017/09/04 18:13
Subject:        Re: RFR: 8187033: [PPC] Imporve performance of 
ObjectStreamClass.getClassDataLayout()


Hi Peter, 

Thank you for fixing the code.

I updated webrev: http://cr.openjdk.java.net/~horii/8187033/webrev.02/

Regards,
Ogata


Peter Levart <peter.levart at gmail.com> wrote on 2017/09/04 17:45:37:

> From: Peter Levart <peter.levart at gmail.com>
> To: Kazunori Ogata <OGATAK at jp.ibm.com>
> Cc: core-libs-dev <core-libs-dev at openjdk.java.net>, Hans Boehm 
> <hboehm at google.com>, Aleksey Shipilev <shade at redhat.com>
> Date: 2017/09/04 17:45
> Subject: Re: RFR: 8187033: [PPC] Imporve performance of 
> ObjectStreamClass.getClassDataLayout()
> 
> Hi Ogata,
 
<snip>

> If playing with mutable plain fields in multiple threads, it is 
> mandatory to read the plain field just once in program. Your 
implementation:
> 
> 1196     ClassDataSlot[] getClassDataLayout() throws 
InvalidClassException {
> 1197         // REMIND: synchronize instead of relying on volatile?
> 1198         if (dataLayout == null) {
> 1199             ClassDataSlot[] slots = getClassDataLayout0();
> 1200             VarHandle.fullFence();
> 1201             dataLayout = slots;
> 1202         }
> 1203         return dataLayout;
> 1204     }
> 
> reads dataLayout field twice (line 1198 and 1203). Those two reads may 
> reorder and 1st return non-null value, while the 2nd return previous 
> value - null. You should use a local variable to store the 1st read and 
> return the local variable at the end. Like:
> 
>       ClassDataSlot[] getClassDataLayout() throws InvalidClassException 
{
>           ClassDataSlot[] slots = dataLayout;
>           if (slots == null) {
>               ClassDataSlot[] slots = getClassDataLayout0();
>               VarHandle.fullFence();
>               dataLayout = slots;
>           }
>           return slots;
>       }
> 
> 
> Regards, Peter
>