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

[Stefan Karlsson]

On 09/10/2013 10:25 AM, Mikael Gerdin wrote:
> Hi,
>
> can I have a second review for this change?
>
> The latest webrev is at:
> http://cr.openjdk.java.net/~mgerdin/8009561/webrev.3/

http://cr.openjdk.java.net/~mgerdin/8009561/webrev.3/src/share/vm/memory/metaspace.cpp.frames.html

This should be using >= instead of >:

2330 void SpaceManager::retire_current_chunk() {
2331   if (current_chunk() != NULL) {
2332     size_t remaining_words = current_chunk()->free_word_size();
2333     if (remaining_words > TreeChunk<Metablock, FreeList>::min_size()) {
2334       block_freelists()->return_block(current_chunk()->allocate(remaining_words), remaining_words);
2335       inc_used_metrics(remaining_words);
2336     }
2337   }
2338 }

otherwise this looks good to me.

If you're going to edit this patch, maybe you could split this line into 
two and capitalize the start of the sentence?

  228   // only allocate and split from freelist if the size of the allocation is at least 1/4th the size of the available block
  229   const static int WasteMultiplier = 4;


thanks,
StefanK

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