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

[Jiangli Zhou]

Thanks, John!

Jiangli

On 06/20/2012 05:20 PM, John Coomes wrote:
> 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