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

[David Holmes]

Hi Jiangli,

Nit: You don't need to call has_generic_signature() inside 
generic_signature() if the caller already does it. Conversely if you do 
call it then the caller need not. I prefer to see the caller do the 
check (or choose not to). But I won't push this issue.

The same style of API change can be applied to method 
generic_signatures, and should be for consistency. But this can be 
handled by a further RFE.

Thanks,
David

On 21/06/2012 8:52 AM, Jiangli Zhou 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/
>> 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
>