Request for review 7177409: Perf regression in JVM_GetClassDeclaredFields after generic signature changes

[John Coomes]

Jiangli Zhou (jiangli.zhou at oracle.com) wrote:
> Hi John,
> 
> On 06/20/2012 03:10 PM, John Coomes wrote:
> > Jiangli Zhou (jiangli.zhou at oracle.com) wrote:
> >> Hi David,
> >>
> >> Here is the updated weberv:
> >>
> >>     http://cr.openjdk.java.net/~jiangli/7177409/webrev.01/
> >    41 Symbol* fieldDescriptor::generic_signature() const {
> >    42   if (!access_flags().field_has_generic_signature()) {
> >    43     return NULL;
> >    44   }
> >    45
> >
> > Nit: you should use the new has_generic_signature() method here, so it
> > becomes
> >
> >         if (!has_generic_signature()) {
> >         ...
> Yes, thanks for the suggestion! New webrev: 
> http://cr.openjdk.java.net/~jiangli/7177409/webrev.02/

Looks good to me.

-John

> > Otherwise looks good.
> 
> Thanks for your review.
> 
> Jiangli
> 
> > -John
> >
> >> On 06/19/2012 08:13 PM, Jiangli Zhou wrote:
> >>> Hi David,
> >>>
> >>> Thanks for the review! Please see comments below.
> >>>
> >>> On 06/19/2012 06:41 PM, David Holmes wrote:
> >>>> HI Jiangli,
> >>>>
> >>>> On 20/06/2012 3:32 AM, Jiangli Zhou wrote:
> >>>>> Please review the following webrev that address the performance
> >>>>> regression in JVM_GetClassDeclaredFields with very large class.
> >>>>>
> >>>>> http://cr.openjdk.java.net/~jiangli/7177409/webrev.00/
> >>>>>
> >>>>> In fieldDescriptor::generic_signature(), it now returns NULL
> >>>>> immediately
> >>>>> if the access_flags indicates the field has no generic signature. This
> >>>>> avoids iterating though the fields unnecessarily. Mikael Gerdin has
> >>>>> confirmed the change resolves the performance issue for
> >>>>> JVM_GetClassDeclaredFields (thanks, Mikael!).
> >>>> Your change fixes the problem but I can't help but feel that there is
> >>>> an API problem here, and a usage problem in that this fragment in
> >>>> reflection.cpp:
> >>>>
> >>>>    if (java_lang_reflect_Field::has_signature_field()&&
> >>>>        fd->generic_signature() != NULL) {
> >>>>      Symbol*  gs = fd->generic_signature();
> >>>>
> >>>> should really be:
> >>>>
> >>>>    if (java_lang_reflect_Field::has_signature_field()&&
> >>>>        fd->has_generic_signature()) {
> >>>>      Symbol*  gs = fd->generic_signature();
> >>>>
> >>>> If the query is basically constant time the performance would not be
> >>>> impacted. In fact the current code seems to be relying on the C
> >>>> compiler to reuse the result of generic_signature() so that it
> >>>> doesn't actually call it twice. If it is calling it twice then an API
> >>>> change (for methods too) would yield an even better performance
> >>>> improvement.
> >>> Yes, most compiler probably would not generate code to reload the
> >>> fd->generic_signature(). But adding an has_generic_signature() API and
> >>> use here seems good to me too. I can make that along with the bug fix.
> >>>
> >>> Thanks,
> >>>
> >>> Jiangli
> >>>
> >>>> Maybe an RFE ...
> >>>>
> >>>> David
>