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

[Coleen Phillimore]

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

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
>