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

[John Coomes]

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()) {
       ...

Otherwise looks good.

-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
> >
>