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

[Roger Riggs]

Hi,

The repo's are closed in the short term for the consolidation.

It seems like the consensus is that full fence is the best solution.

I can sponsor the issue when the jdk 10 repos re-opens.

Regards, Roger


On 9/14/2017 5:15 AM, Kazunori Ogata wrote:
> 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
>>
>
>