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

[Jon Masamitsu]

On 10/14/13 11:02 PM, Stefan Karlsson wrote:
> 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.
Thanks (for name change) and OK (on metachunk files).

Jon
>
> 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
>>>>
>>>
>>
>