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

[Kazunori Ogata]

Hi Aleksey,

The previous code won't work.  It should be like this:


private ClassDataSlot[] dataLayout;
private volatile ClassDataSlot[] dataLayout_tmp;

ClassDataSlot[] getClassDataLayout() throws InvalidClassException {
  if (dataLayout == null) {
    dataLayout_tmp = getClassDataLayout0();
    dataLayout = dataLayout_tmp;
  }
  return dataLayout;
}


Regards,
Ogata




From:   Kazunori Ogata/Japan/IBM
To:     Aleksey Shipilev <sh...@redhat.com>
Cc:     core-libs-dev@openjdk.java.net
Date:   2017/08/31 20:12
Subject:        Re: RFR: 8187033: [PPC] Imporve performance of 
ObjectStreamClass.getClassDataLayout()


Hi Aleksey,

Thank you for your comment.  I understand your point that a memory fence 
is needed between writes to slots of ClassDataSlot[] and the reader of 
$dataLayout.

Then I think we can put a fence only when dataLayout is updated in the 
following way:


private ClassDataSlot[] dataLayout;

ClassDataSlot[] getClassDataLayout() throws InvalidClassException {
  if (dataLayout == null) {
    volatile ClassDataSlot dl = getClassDataLayout0();
    dataLayout = dl;
  }
  return dataLayout;
}


Is this correct and acceptable?


Regards,
Ogata




From:   Aleksey Shipilev <sh...@redhat.com>
To:     Kazunori Ogata <oga...@jp.ibm.com>, core-libs-dev@openjdk.java.net
Date:   2017/08/31 19:29
Subject:        Re: RFR: 8187033: [PPC] Imporve performance of 
ObjectStreamClass.getClassDataLayout()



On 08/31/2017 12:05 PM, Kazunori Ogata wrote:
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8187033
> Webrev: http://cr.openjdk.java.net/~horii/8187033/webrev.00/

Alas, this change is incorrect. It introduces the race between readers and 
writers of dataLayout
field. Which means, whatever writers have done in ClassDataSlot[] array is 
not guaranteed to be
visible to the readers of $dataLayout.

You can argue this race is benign, but it is not. ClassDataSlot has only 
the final fields, which is
handy. But the problem is with the ClassDataSlot[] array itself: its 
elements are not "final". Maybe
doing the wrapper with final field around it would help, at the expense of 
additional dereference, e.g.:

private DataLayout dataLayout;

class DataLayout {
  final ClassDataSlot[] slots;  // "final" is important here
  DataLayout(List<ClassDataSlot> src) {
    this.slots = src.toArray(new ClassDataSlot[src.size()];
  }
}

ClassDataSlot[] getClassDataLayout() throws InvalidClassException {
  if (dataLayout == null) {
    dataLayout = getClassDataLayout0();
  }
  return dataLayout.slots;
}

Plus maybe replace all declarations of ObjectStreamClass.ClassDataSlot[] 
with
ObjectStreamClass.DataLayout after this...

Thanks,
-Aleksey

[attachment "signature.asc" deleted by Kazunori Ogata/Japan/IBM]