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

[Mikael Gerdin]

Thanks Jon! 

/Mikael 

Jon Masamitsu <jon.masamitsu at oracle.com> wrote:

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