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

[Jiangli Zhou]

Hi David,

On 06/20/2012 08:32 PM, David Holmes wrote:
> 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.

I agree with your point about the has_generic_signature() check. Adding 
the has_generic_signature() check in generic_signature() allows it 
return NULL immediately if the requested field has no generic signature. 
That cuts down the overhead of having to iterating through the fields 
before the requested one, which was where the most of the performance 
regression came from. It also does not need any special check/handling 
in the caller and does not need change in any of the existing call 
sites. In the JVM_GetClassDeclaredFields() case, it already has the 
check before calling generic_signature(), changing the logic of 
JVM_GetClassDeclaredFields() is probably beyond the scope of the bug fix.

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

Yes.

Thanks,

Jiangli

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