Request for review: JDK-8009561 NPG: Metaspace fragmentation when retiring a Metachunk

[Jon Masamitsu]

Looks good.

Jon

On 8/29/13 5:30 AM, Mikael Gerdin wrote:
> Jon,
>
> Updated webrev at:
> http://cr.openjdk.java.net/~mgerdin/8009561/webrev.3
>
> I also noticed that in the code you mentioned:
>
>  825    size_t unused = block_size - word_size;
>  824   if (unused > TreeChunk<Metablock, FreeList>::min_size()) {
>  826     return_block(new_block + word_size, unused);
>  827   }
>
> that the ">" check is incorrect. It should in fact be a ">=" since a 
> block of min_size() is fine as well.
>
> /Mikael
>
> On 2013-08-29 11:11, Mikael Gerdin wrote:
>> Jon,
>>
>> On 2013-08-16 15:59, Jon Masamitsu wrote:
>>> Mikael,
>>>
>>> 1) Should we not split the block if the remainder is not larger than
>>> the minimum size for the dictionary?  Just return the block to the
>>> dictionary and fail the allocation?
>>
>> I think that would reduce the usefulness of the free list. It's
>> unavoidable to waste a few words here and there due to alignment and
>> such. If the allocation request is close enough in size I think we
>> should take the chance to reuse the memory from the free list.
>>
>>>
>>> 2) Could you reuse "unused" like this.
>>>
>>>   825    size_t unused = block_size - word_size;
>>>   824   if (unused > TreeChunk<Metablock, FreeList>::min_size()) {
>>>   826     return_block(new_block + word_size, unused);
>>>   827   }
>>
>> Sure, I'll fix that.
>>
>>>
>>>
>>> 3) Your change to reduce the threshold is a good one.  Was this 
>>> motivated
>>> by some behavior you observed.
>>
>> It was more of a hunch. 64k in the free list per
>> Metaspace/CLD/ClassLoader seemed excessive. I can't say that I've done
>> any scientific experiments to show that 4k is better but I think it's
>> good enough.
>>
>> /Mikael
>>
>>>
>>> Thanks.
>>>
>>> Jon
>>>
>>>
>>> On 8/14/2013 5:59 AM, Mikael Gerdin wrote:
>>>> Hi,
>>>>
>>>> After some discussions and some benchmarking I have a new version of
>>>> the change:
>>>>
>>>> http://cr.openjdk.java.net/~mgerdin/8009561/webrev.1/
>>>>
>>>> The idea is to use FreeBlockDictionary<Metablock>::atLeast but
>>>> limiting the splitting of large blocks by refusing the allocation if
>>>> the block returned from the freelist is 4x larger than the allocation
>>>> request.
>>>>
>>>> I also reduced the freelist allocation threshold since 64k seems too
>>>> large for what is in effect a per-ClassLoaderData freelist.
>>>>
>>>> /Mikael
>>>>
>>>> On 2013-06-05 16:04, Mikael Gerdin wrote:
>>>>> Hi,
>>>>>
>>>>> Can I have some reviews of this small fix to the Metaspace memory
>>>>> allocation path.
>>>>>
>>>>> Problem:
>>>>> When a Metaspace allocation request cannot be satisfied by the 
>>>>> current
>>>>> chunk the chunk is retired and a new chunk is requested. This causes
>>>>> whatever is left in the chunk to be effectively leaked.
>>>>>
>>>>> Suggested fix:
>>>>> Put the remaining memory in each chunk on the Metablock freelist 
>>>>> so it
>>>>> can be used to satisfy future allocations.
>>>>>
>>>>> Possible addition:
>>>>> When allocating from the block free list, use
>>>>> FreeBlockDictionary<Metablock>::atLeast instead of
>>>>> FreeBlockDictionary<Metablock>::exactly and split the Metablock if 
>>>>> it's
>>>>> large enough.
>>>>>
>>>>> One might argue that this increases the fragmentation of the 
>>>>> memory on
>>>>> the block free list but I think that we primarily want to use the 
>>>>> block
>>>>> free list for small allocations and allocate from chunks for large
>>>>> allocations.
>>>>>
>>>>> Webrev:
>>>>> Only fix:
>>>>> http://cr.openjdk.java.net/~mgerdin/8009561/webrev.0/
>>>>>
>>>>> Incremental webrev for splitting blocks:
>>>>> http://cr.openjdk.java.net/~mgerdin/8009561/webrev.0%2b/
>>>>>
>>>>> Bug links:
>>>>> https://jbs.oracle.com/bugs/browse/JDK-8009561
>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8009561
>>>>>
>>>>> Thanks
>>>>> /Mikael
>>>