Re: 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.lev...@gmail.com>
Cc:     core-libs-dev <core-libs-dev@openjdk.java.net>, Hans Boehm
<hbo...@google.com>, Aleksey Shipilev <sh...@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.lev...@gmail.com> wrote on 2017/09/04 17:45:37:

From: Peter Levart <peter.lev...@gmail.com>
To: Kazunori Ogata <oga...@jp.ibm.com>
Cc: core-libs-dev <core-libs-dev@openjdk.java.net>, Hans Boehm
<hbo...@google.com>, Aleksey Shipilev <sh...@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