RFR: JDK-8229871: Improve performance of Method.copy() and leafCopy()

[Kazunori Ogata]

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.chung at oracle.com> wrote on 2019/10/09 04:24:35:

> From: Mandy Chung <mandy.chung at oracle.com>
> To: Claes Redestad <claes.redestad at oracle.com>, Kazunori Ogata 
<OGATAK at jp.ibm.com>
> Cc: core-libs-dev at 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
>