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

[Coleen Phillimore]

This looks good.
Thanks,
Coleen

On 6/20/2012 5:10 PM, Jiangli Zhou wrote:
> Hi David,
>
> Here is the updated weberv:
>
>   http://cr.openjdk.java.net/~jiangli/7177409/webrev.01/
>
> Thanks,
> Jiangli
>
> 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
>>
>