Request for review: 7168280: Eliminate the generic signature index slot from field array for field without generic signature

[Dean Long]

On 5/14/2012 10:48 AM, Jiangli Zhou wrote:
> Hi Dean,
>
> Thanks for the further comments.
>
> On 05/11/2012 06:32 PM, Dean Long wrote:
>> How about using a typeArrayOop as your temporary storage in 
>> parse_fields() instead of a u2 array?  That way you can use the 
>> existing from_field_array() that does a bounds check.
>
> Several approaches were attempted for parse_fields(). Initially a 
> typeArrayOop was used. However measurements indicated memory increase 
> instead of reduction with the approach. The temporary allocated 
> typeArrayOops were not being GC'ed and caused memory leaks. As a 
> second approach, the number of fields with generic signature was 
> counted first so a typeArrayOop was allocated with the exact size. 
> That approached required an extra iteration of all fields. The 
> temporary resource array approach avoids the pre-parsing of all fields 
> and also does not have memory leak.
>
>>
>> It is unfortunate that FieldInfo can't get generic signature 
>> information anymore, forcing you to move more logic into the callers.
>
> Fortunately most of the logic are hidden in the fieldStreams.
>
>>
>> Some of the code that iterates through all the fields could be more 
>> efficient if you still had all_fields_count() so you could easily 
>> find the generic signature information at the end.  One way to do 
>> that is to store the total fields count at the end of the array only 
>> if the array contains generic signature information.  You can tell if 
>> the array contains generic signature information by the length.  If 
>> the length % FieldInfo::field_slots == 0, then there is no generic 
>> signature information, and all_fields_count() is computed like 
>> before.  If length % FieldInfo::field_slots != 0 then get the fields 
>> count from the end of the array.  You would need to pad the array 
>> with an extra slot if the number of generic signature slots happened 
>> to be a multiple of FieldInfo::field_slots.
>
> I considered of recording the field count in the array. However it 
> still uses extra 2 bytes for each class. I also played with the idea 
> of using length % FieldInfo::field_slots as indicator of the existence 
> of generic signatures. But if the number of fields is the multiply of 
> FieldInfo::field_slots, length % FieldInfo::field_slots can be 0 if 
> all fields have generic signatures.
>

Right, that's why I said you would need to pad the array with an extra slot.

> I did some performance measurements with the field array changes. 
> There seems no performance degradation.
>
> ============================================================================== 
>
> logs.field_baseline.1:
>   Benchmark           Samples        Mean     Stdev             
> Geomean Weight
>   specjbb2005               8    56057.48    454.04
>   specjvm98                 8      533.28     17.25
> ============================================================================== 
>
> logs.field.2:
>   Benchmark           Samples        Mean     Stdev   %Diff     P  
> Significant
>   specjbb2005               8    56321.18    614.18    0.47 
> 0.347            *
>   specjvm98                 8      537.81     11.07    0.85 
> 0.544            *
> ============================================================================== 
>
>
> Thanks again for the review and comments. Can I count you as a reviewer?
>

OK.

dl

> Jiangli
>
>>
>> dl
>>
>> On 5/11/2012 2:43 PM, Jiangli Zhou wrote:
>>> Hi Dean,
>>>
>>> Here is the updated webrev 
>>> http://cr.openjdk.java.net/~jiangli/7168280/webrev.01/. In 
>>> init_generic_signature_start_slot(), it first scans from f[0] to 
>>> f[_index - 1] so the generic signature slots for those fields can be 
>>> skipped while scanning from f[_index] to the end.
>>>
>>> Thanks again,
>>>
>>> Jiangli
>>>
>>> On 05/11/2012 01:53 PM, Jiangli Zhou wrote:
>>>> Hi Dean,
>>>>
>>>> Good catch! Thanks for the comments.
>>>>
>>>> Jiangli
>>>>
>>>> On 05/11/2012 01:38 PM, Dean Long wrote:
>>>>> I think this breaks InternalFieldStream. The loop in 
>>>>> init_generic_signature_start_slot() will start at 
>>>>> java_fields_count() instead of 0, so it won't decrement "length" 
>>>>> for any Java fields with a generic signature, resulting in 
>>>>> scanning too many elements.
>>>>>
>>>>> dl
>>>>>
>>>>> On 5/11/2012 12:02 PM, Jiangli Zhou wrote:
>>>>>> Hi,
>>>>>>
>>>>>> The field array is an array of 7-shorts [access, name index, sig 
>>>>>> index, initval index, low offset, high offset, generic signature 
>>>>>> index]. Most fields don't have generic signature attribute, in 
>>>>>> that case the generic signature index in the field array is 0 and 
>>>>>> unused. For all classes in rt.jar, there are total about 28973 
>>>>>> fields. From which 27466 fields do not have generic signature and 
>>>>>> only 1507 fields have generic signature.
>>>>>>
>>>>>> Following is a webrev that eliminates the unused generic 
>>>>>> signature index slot from field array. For field without generic 
>>>>>> signature, the field data is 6-shorts. For field with generic 
>>>>>> signature, it is still 7-shorts of data. A new flag is added in 
>>>>>> the field access flag to indicate if the field has the extra 
>>>>>> short for generic signature index.
>>>>>>
>>>>>>   http://cr.openjdk.java.net/~jiangli/7168280/webrev.00/
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Jiangli
>>>>
>>>
>