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

[Jiangli Zhou]

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