Hi Mandy, Thank you for review.
I'll keep the constructor package-private, as the change will be non-trivial when I fix the code based on the Peter's comment. Regards, Ogata Mandy Chung <mandy.ch...@oracle.com> wrote on 2019/10/09 04:24:35: > From: Mandy Chung <mandy.ch...@oracle.com> > To: Claes Redestad <claes.redes...@oracle.com>, Kazunori Ogata <oga...@jp.ibm.com> > Cc: core-libs-dev@openjdk.java.net > Date: 2019/10/09 04:24 > Subject: [EXTERNAL] Re: RFR: JDK-8229871: Improve performance of > Method.copy() and leafCopy() > > > > On 10/8/19 6:31 AM, Claes Redestad wrote: > > Hi, > > > > webrev.02 looks good to me. > > > > webrev.02 looks fine to me as well. > > > I think the performance results makes sense since avoiding a volatile > > store (and the potentially expensive store barriers this involves) > > should be the main benefit. Adding a branch to avoid storing null would > > help partially, but not hot Methods. > > > > Pre-existing issue, but it's somewhat weird that we have two assignments > > outside the package-private constructor: adding root and methodAccessor > > to the constructor would make more immediate sense to me, since we do > > the same thing at the only two callsites: > > > > Method res = new Method(clazz, name, parameterTypes, returnType, > > exceptionTypes, modifiers, slot, signature, > > annotations, parameterAnnotations, annotationDefault); > > res.root = root; > > res.methodAccessor = methodAccessor; > > > > -> > > > > Method res = new Method(clazz, name, parameterTypes, returnType, > > exceptionTypes, modifiers, slot, signature, > > annotations, parameterAnnotations, annotationDefault, > > root, methodAccessor); > > > > This package-private constructor could also be made private. My guess is > > there was some time when this was used from outside Method and changing > > it was deemed unsavory.. > > The parameters of the Method constructor are the content from a > ClassFile to represent a method whereas root and methodAccessor fields > are not part of it. The current implementation allocates a Method > instance and sets the fields individually instead of calling the > constructor probably due to bootstrapping. > > I suggest to keep it as it is. OTOH making the constructor private is fine. > > Mandy >
