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

[Jon Masamitsu]

On 10/15/13 5:06 AM, Coleen Phillimore wrote:
> On 10/15/2013 2:02 AM, 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.
>
> Sure.
>>
>> I'll do the renaming as a separate patch when we've figured out a 
>> good name and file separation.
>
> I don't think this looks so bad as Metablock and Metachunk are mostly 
> internal to metaspace anyway. 
I don't insist on a file separation change.
> Rename MetaspaceAux first!

Give us a new name! :-)

Jon

>
> Thanks,
> Coleen
>
>>
>> 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
>>>>>
>>>>
>>>
>>
>