Review request 7109878: The instanceKlass EnclosingMethhod attribute fields can be folded into the _inner_class field

[Jiangli Zhou]

Here is the updated webrev that incorporates Tom's suggestions: 
http://cr.openjdk.java.net/~jiangli/7109878/webrev.02/.

Thanks!

Jiangli

On 02/24/2012 05:12 PM, Jiangli Zhou wrote:
> Hi Tom,
>
> Thanks very much for your review and comments.
>
> On 02/24/2012 04:23 PM, Tom Rodriguez wrote:
>> On Feb 21, 2012, at 10:41 AM, Jiangli Zhou wrote:
>>
>>> The updated webrev is 
>>> http://cr.openjdk.java.net/~jiangli/7109878/webrev.01/.
>> How does the math work out for whether this is a net win on space?  
>> If you have a bunch of klasses with EnclosingMethod attributes but no 
>> inner classes then you are allocating an extra Java array just to 
>> store 2 u2s.  For code that makes heavy use of inner classes might 
>> this cause it to take more space, particularly in 64 bit where the 
>> header is larger?
>
> That's very good question. In the VM spec 4.8.5, it states "If the 
> constant pool of a class or interface C con-tains a 
> CONSTANT_Class_info entry which represents a class or interface that 
> is not a member of a package, then C‘s ClassFile structure must have 
> exactly one InnerClasses attribute in its attributes table." For each 
> inner class (not nested), javac generates an InnerClasses attribute 
> with one entry ('number_of_classes' is 1) in the 'classes' array. The 
> entry refers the inner class itself. A local or anonymous class is 
> inner class and always has a InnerClasses attribute. So there is no 
> wasting of memory by appending the EnclosingMethod related data to the 
> inner_classes array, since the array already exists for the class.
>
>> Did you consider making the inner_classes and enclosing methods 
>> arrays be embedded in the variable sized portion of the 
>> instanceKlass, following the nonstatic_oop_maps?  That would save a 
>> pointer for every class as well as reducing the overhead for inner 
>> classes array itself.
>
> Haven't thought about that. Thanks for the suggestion! I'm making the 
> _implementors array and _nof_implementors as embedded fields 
> currently. I'll look into the inner_classes array together with those 
> two fields.
>> In instanceKlass.cpp, you seem to be using EXCEPTION_MARK to get 
>> THREAD defined and that's not really its purpose.  You don't even 
>> need THREAD defined in that code you can just use the constructor 
>> that doesn't take a thread argument and it will fetch the current 
>> thread.
>
> Ok.
>
>> I think it might be worth hiding the inner_classes array behind an 
>> iterator class instead of allowing direct access to the array and 
>> modifying all the iterators to use the new bounds check logic.  It 
>> just seems like it's asking for bugs.  Hiding it like this would be 
>> necessary if the array were embedded in the klass.
>
> That sounds good idea. I'll try to make a iterator.
>
>> You'll also need to modify the SA handle this.
>
> Ok.
>
> Thanks!
>
> Jiangli
>> tom
>>
>>> Thanks,
>>>
>>> Jiangli
>>>
>>> On 02/17/2012 06:02 PM, Jiangli Zhou wrote:
>>>> Hi Dean,
>>>>
>>>> Thanks for catching that. Yes, it needs to be bigger than u2 since 
>>>> "length" is already u2. I'll fix it to use int.
>>>>
>>>> Thanks!
>>>>
>>>> Jiangli
>>>>
>>>> On 02/17/2012 05:44 PM, Dean Long wrote:
>>>>> In parse_classfile_inner_classes_attribute():
>>>>>
>>>>> !   u2 size = length * 4 + (parsed_enclosingmethod_attribute ? 2 : 
>>>>> 0);
>>>>>
>>>>> don't you need a size bigger than u2 for "size"?  I think any 
>>>>> value of "length" greater than 0x3fffffff will
>>>>> cause an overflow.
>>>>>
>>>>> dl
>>>>>
>>>>> On 2/17/2012 3:42 PM, Jiangli Zhou wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review the changes that fold the 
>>>>>> instanceKlass::_enclosing_method_class_index and 
>>>>>> instanceKlass::_enclosing_method_method_index into the 
>>>>>> instanceKlass::_inner_classes array. The 
>>>>>> instanceKlass::_enclosing_method_class_index and 
>>>>>> instanceKlass::_enclosing_method_method_index are only used if 
>>>>>> the class contains the EnclosingMethod attribute (local class or 
>>>>>> anonymous class). For majority of the classes, these two fields 
>>>>>> (4byte total) are wasted. Folding them into the _inner_classes 
>>>>>> array and only allocating the space for them saves 4byte for most 
>>>>>> of the loaded classes. The enclosing method indexes are located 
>>>>>> at the end of the _inner_classes array when exist.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jiangli/7109878/webrev.00/
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Jiangli
>