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

[Jon Masamitsu]

On 7/26/2013 6:22 AM, Mikael Gerdin wrote:
> Jon,
>
> On 06/12/2013 12:51 AM, Jon Masamitsu wrote:
>>
>> On 6/11/13 2:46 PM, Mikael Gerdin wrote:
>>> Jon
>>>
>>> On 06/07/2013 07:36 PM, Jon Masamitsu wrote:
>>>>
>>>> On 6/7/2013 8:28 AM, Mikael Gerdin wrote:
>>>>> Jon,
>>>>>
>>>>>
>>>>> On 2013-06-06 16:50, Jon Masamitsu wrote:
>>>>>> Mikael,
>>>>>>
>>>>>> Thanks.  I'd be interested in seeing the instrumentation you
>>>>>> add.  Might be worth adding as an enhancement in a later
>>>>>> changeset.
>>>>>
>>>>> I did a 1hr KS run today with and without block splitting, here's 
>>>>> what
>>>>> I came up with (in an entirely non-scientific way)
>>>>>
>>>>> http://cr.openjdk.java.net/~mgerdin/8009561/splitting.txt
>>>>> http://cr.openjdk.java.net/~mgerdin/8009561/splitting.png
>>>>
>>>> Good graphs.
>>>>
>>>> The behavior is what we expect (I think).  With splitting we are 
>>>> able to
>>>> do more
>>>> small allocations from the dictionary (where we split a larger 
>>>> block to
>>>> get a smaller
>>>> block) and get fewer larger blocks allocated (some have been split).
>>>>
>>>>
>>>>>
>>>>> We hit the HWM 4 times with splitting and 5 times without splitting.
>>>>
>>>> Because we don't have to expand (get new chunks as often, which is
>>>> good) I
>>>> would surmise.
>>>>
>>>>> On the other hand: splitting did lead us with more metaspace memory
>>>>> committed in the end.
>>>>
>>>> One explanation would be that allocations of larger block need to come
>>>> out of newly committed space instead of the dictionary (where the 
>>>> large
>>>> blocks have been broken up).
>>>>
>>>> Is there a policy that we could use that says
>>>>
>>>> "break up a larger block for a smaller block allocation only if ..."
>>>>
>>>> You fill in the blank?
>>>
>>> ...only if the larger block is less than 4 times larger than the
>>> allocation? 2 times? 8 times?
>>>
>>> I could try some more KS runs but I'm unsure if the figures I come up
>>> with are actually relevant.
>>
>> I also don't know if more KS runs would be relevant.    Can you ask the
>> dictionary
>> how many blocks there are of the size you're going to split?  If we only
>> split if
>> there are more than 4 blocks of that size, that would moderate the
>> splitting
>> a bit.
>
> I thought about this again and when I saw Fred's patch I think that 
> not splitting blocks won't help us here.
> As Fred noticed when we deallocate we deallocate based on the known 
> size of an object. So even if I don't split in the allocation path the 
> rest of the Metablock returned from the freelist is wasted because if 
> it is deallocated the deallocation path has no knowledge of the size 
> of the block originally returned from the block freelist, right?

When you get a Metablock from the freelist, you know its size 
(accurately after Fred's change).
Then you can calculate the size of the Metablock that is being put back 
on the freelist.
I don't see a problem.

Jon

>
> I should probably redo my runs with Fred's patch applied as well.
>
> /Mikael
>
>>
>> Jon
>>>
>>>
>>>>
>>>>
>>>>>
>>>>> I put up the very simple instrumentation at:
>>>>> http://cr.openjdk.java.net/~mgerdin/8009561/instrument/webrev
>>>>>
>>>>> I also changed the allocation_from_dictionary_limit to 4k to force us
>>>>> to make more freelist allocations.
>>>>
>>>> Does it really make sense to have any 
>>>> allocation_from_dictionary_limit?
>>>> I know it was initially added because allocation from a freelist takes
>>>> longer
>>>> but to have a static limit like that just seems to put that space
>>>> forever
>>>> beyond reach.
>>>
>>> I thought you had added the limit. I sort of feel that 64k is a bit
>>> much but the code would definitely be simpler if there was none.
>>> We already take the hit of acquiring a Mutex for each Metaspace
>>> allocation so maybe the dictionary lookup isn't that expensive?
>>>
>>>>
>>>> Thanks for the numbers.
>>>
>>> You're welcome.
>>>
>>> /Mikael
>>>
>>>>
>>>> Jon
>>>>
>>>>>
>>>>> /Mikael
>>>>>
>>>>>>
>>>>>> Jon
>>>>>>
>>>>>>
>>>>>> On 6/6/13 2:22 AM, Mikael Gerdin wrote:
>>>>>>> Jon,
>>>>>>>
>>>>>>> On 2013-06-06 04:41, Jon Masamitsu wrote:
>>>>>>>>
>>>>>>>> On 6/5/2013 7:04 AM, 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/
>>>>>>>>
>>>>>>>> The "Only fix" looks good.  Did you test with
>>>>>>>> metaspace_slow_verify=true?
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Incremental webrev for splitting blocks:
>>>>>>>>> http://cr.openjdk.java.net/~mgerdin/8009561/webrev.0%2b/
>>>>>>>>
>>>>>>>> Change looks good.
>>>>>>>>
>>>>>>>> Did you do any long running tests with the block splitting? 
>>>>>>>> Such as
>>>>>>>> 24hours with kitchensink?  Something that would reuse Metablocks
>>>>>>>> so that we can see if we are fragmenting instead of reusing?
>>>>>>>>
>>>>>>>
>>>>>>> I did some runs earlier but I don't have any data from them.
>>>>>>> I can try to get an instrumented build together and run KS over the
>>>>>>> weekend.
>>>>>>>
>>>>>>> /Mikael
>>>>>>>
>>>>>>>> Jon
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Bug links:
>>>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8009561
>>>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8009561
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> /Mikael
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>
>