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

[Jiangli Zhou]

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