Review request: 8026391: The Metachunk header wastes memory - 8026392: Metachunks and Metablocks are using a too large alignment

[Stefan Karlsson]

On 2013-10-15 00:46, Jon Masamitsu wrote:
>
> On 10/14/13 2:31 PM, Coleen Phillimore wrote:
>>
>> I think this looks good but I didn't review it completely.  I had the 
>> opposite request as Jon though.  I think "Metamem" is odd and if 
>> there's no reason to abbreviate something used only a couple places.  
>> Can you name the class Metamemory?  And leave the file name 
>> metachunk?  We have a lot of examples where the base class is not the 
>> same as the name of the file and the file is named after the class 
>> that the JVM uses outside the file (eg. growableArray.hpp).
>
> I would go along with that except that Metablock is in the same file so
> you have 2 VM visible types in the same file that is named after one
> of them.   I'll grant you the longer name Metamemory.  I think it's a
> little ugly.  Metabase?

I'll change Metamem to Metabase and leave the three classes in the 
metachunk files, for now.

I'll do the renaming as a separate patch when we've figured out a good 
name and file separation.

thanks!
StefanK
>
> Jon
>
>>
>> Now I've reviewed it.  I think the Metamemory refactoring.
>>
>> Thanks,
>> Coleen
>>
>>
>> On 10/14/2013 04:40 PM, Jon Masamitsu wrote:
>>> Comments only on the first patch.
>>>
>>> Looks good.  Couple of questions.
>>>
>>> Why the change from _is_free to _is_marked_free?
>>>
>>> From
>>>
>>>   58   bool _is_free;
>>>
>>> to
>>>
>>>  105   DEBUG_ONLY(bool _is_marked_free;)
>>>
>>>
>>> Is it to specifically distinguish it from the FreeChunk is_free()?
>>>
>>> The "marked" in the name feels a little like GC when it
>>> isn't GC related.  Would "_is_tagged_free" make sense?
>>>
>>>
>>> Did you consider changing the file names from Metachunk.* to
>>> Metamem.*?  The file contains the classes Metamem, Metablock,
>>> and Metachunk.  It sees more natural to me to have the file named
>>> after the more general Metamem class.  Then all the classes in
>>> the file are Metamem (or derived from), yes?
>>>
>>> Jon
>>>
>>> On 10/14/13 4:58 AM, Stefan Karlsson wrote:
>>>> Please, review these two patches to remove some of the wastages 
>>>> introduced in the metaspace.
>>>>
>>>>
>>>> 8026391: The Metachunk header wastes memory
>>>>  http://cr.openjdk.java.net/~stefank/8026391/webrev.00/
>>>>  https://bugs.openjdk.java.net/browse/JDK-8026391
>>>>
>>>> There are a couple of fields in Metachunk that store already known 
>>>> information, like the beginning and the end of the Metachunk. I 
>>>> replaced those fields with code instead.
>>>>
>>>> To make this easier, I extracted some duplicated code in Metablock 
>>>> and Metachunk into a super class named Metamem. The extracted code 
>>>> is purely needed to allow these objects to be placed on the 
>>>> FreeList and in the BinaryTreeDictionary.
>>>>
>>>>
>>>> 8026392: Metachunks and Metablocks are using a too large alignment
>>>>  http://cr.openjdk.java.net/~stefank/8026392/webrev.00/
>>>>  https://bugs.openjdk.java.net/browse/JDK-8026392
>>>>
>>>> The metaspace code was using ARENA_ALIGN to align the Metablocks 
>>>> and Metachunk header. This forced all allocations of metadata to be 
>>>> 16 bytes aligned. This patch patch changes the restriction to be 8 
>>>> bytes instead, since that's the restriction needed for 64 bit types 
>>>> like long, double, pointers and klass pointers,
>>>>
>>>>
>>>> Testing: Added unit test for Metachunk. JPRT (before splitting of 
>>>> the patches)
>>>>
>>>> thanks,
>>>> StefanK
>>>
>>
>