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

[Jon Masamitsu]

On 8/29/13 2:11 AM, 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.

Ok.  We'll get it all back when the Metaspace is freed.

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