RFR(S) 8015255: NPG: Don't waste fragment at the end of a VirtualSpaceNode before retiring it.

[Stefan Karlsson]

On 10/17/13 5:09 PM, Mikael Gerdin wrote:
> Stefan,
>
> Thanks for looking at this, I've replied to your comments inline.
>
> On Thursday 17 October 2013 16.15.03 Stefan Karlsson wrote:
>> On 2013-10-17 14:56, Mikael Gerdin wrote:
>>> Hi all,
>>>
>>> Please review this change to fix needless memory waste in Metaspace.
>>>
>>> Description of problem:
>>> When receiving an allocation request which cannot be satisfied with the
>>> current VirtualSpaceNode the remaining committed memory in the current
>>> node is wasted when a new node is created and the allocation is satisfied
>>> from the new node.
>>>
>>> Suggested fix:
>>> Before creating a new VirtualSpaceNode, "retire" the current node by
>>> allocating chunks of decreasing size from the committed memory present in
>>> the current node.
>>> To ensure that allocating chunks at the end of a node uses all committed
>>> memory I've increased the size alignment for humongous chunks to the
>>> smallest chunk size. I don't foresee this increasing memory waste since
>>> previously the memory difference between 2-word-aligned sizes and
>>> smallest chunk-aligned would go unused since no chunk would fit there.
>>>
>>> Testing:
>>> Added new unit tests for VirtualSpaceNode and the retire functionality.
>>> JPRT run (including the new unit tests)
>>>
>>> Webrev: http://cr.openjdk.java.net/~mgerdin/8015255/webrev.0/
>>> Buglink: https://bugs.openjdk.java.net/browse/JDK-8015255
>> Very nice.
>>
>> Some comments:
>>
>> Would it make more sense to move the call to
>> retire_current_virtual_space() into create_new_virtual_space() or link_vs()?
> If I do that I would need to first check if there is a current virtual space,
> since create_new_virtual_space() (and and its callee link_vs()) are called
> when a VirtualSpaceList is first created.
> I'd rather keep it in the current place in the code, where I don't need to
> wrap it with if (current_virtual_space() != NULL) {}

I think it makes more sense to retire the current virtual space when 
it's actually being replaced by a new virtual space.

We already have the needed check in link_vs:
void VirtualSpaceList::link_vs(VirtualSpaceNode* new_entry) {
   if (virtual_space_list() == NULL) {
       set_virtual_space_list(new_entry);
   }

But if you don't want to do it I'm not going to fight for it.

>> I think some punctation is missing between the two first lines:
>> +  // If an allocation doesn't fit in the current node a new node is created
>> +  // allocate chunks out of the remaining committed space in this node +
>> // to avoid wasting that memory.
> Fixed
>
>> I think you have an extra "the" in this sentence:
>> +  // This always adds up because the all the chunk sizes are multiples of
>> +  // the smallest chunk size.
> Fixed
>
>> The other usages of HumongousChunkGranularity have to be changed to
>> smallest_chunk_size().
> Fixed, and removed HumongousChunkGranularity
>
>> I had a hard time understanding this test:
>> +    { // 4 pages of VSN is committed, some is used by chunks
>>
>> could you change this line:
>> +      const size_t page_chunks = MIN2(4 * (size_t)os::vm_page_size() /
>> BytesPerWord, vsn_test_size_words);
>>
>> to:
>>          const size_t page_chunks = 4 * (size_t)os::vm_page_size() /
>> BytesPerWord;
>>
>> and add an assert that 4 * (size_t)os::vm_page_size() is less than a
>> MediumChunk (or less than MediumChunk + SmallChunk + SpecializedChunk)?
> Good point, I changed it as you suggested. I added an assert that (page_chunks
> < MediumChunk)
>
> Incremental webrev: http://cr.openjdk.java.net/~mgerdin/8015255/webrev.0-to-1/
> New webrev: http://cr.openjdk.java.net/~mgerdin/8015255/webrev.1/

Great! Looks good.

thanks,
StefanK
>
> /Mikael
>
>> thanks,
>> StefanK
>>
>>> /Mikael