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

[Jiangli Zhou]

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
>