RFR (M) 8164921: Memory leaked when instrumentation.retransformClasses() is called repeatedly

[serguei.spitsyn at oracle.com]

On 10/4/16 07:31, Coleen Phillimore wrote:
>
>
> On 10/3/16 7:11 PM, serguei.spitsyn at oracle.com wrote:
>> Coleen,
>>
>>
>> On 10/3/16 08:46, Coleen Phillimore wrote:
>>>
>>> Serguei, thank you for looking at this.
>>>
>>> On 9/30/16 7:57 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Coleen,
>>>>
>>>>
>>>> Just wanted to share a couple of comments I have so far.
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~coleenp/8164921.01/webrev/src/share/vm/memory/metaspace.cpp.frames.html
>>>>
>>>> 302 debug_only(GrowableArray<Metablock*>* _all_blocks);
>>>> 855 DEBUG_ONLY(_all_blocks = new (ResourceObj::C_HEAP, mtClass) 
>>>> GrowableArray<Metablock*>(100, true)); 863 DEBUG_ONLY(delete 
>>>> _all_blocks;) 891 
>>>> DEBUG_ONLY(_all_blocks->remove((Metablock*)new_block)); 908 
>>>> DEBUG_ONLY(_all_blocks->remove((Metablock*)free_block)); 922 
>>>> DEBUG_ONLY(_all_blocks->remove((Metablock*)new_block));
>>>>   Not clear, why different macros debug_only() is used at L302.   
>>>> Could it be DEBUG_ONLY as well?
>>> Yes, good catch.  I thought I had recompiled after adding the 
>>> DEBUG_ONLY, but I must not have.  It should be DEBUG_ONLY.
>>>>   866 void BlockFreelist::return_block(MetaWord* p, size_t word_size) {
>>>>   867   Metablock* free_chunk = ::new (p) Metablock(word_size);
>>>> 868 if (word_size < SmallBlocks::small_block_min_size()) {
>>>> 869 // Dark matter
>>>> 870 return; 871 } else if (word_size < 
>>>> SmallBlocks::small_block_max_size()) {
>>>>  In case of dark matter the free_chunk is leaked.  It is better to 
>>>> rearrange the fragment to something like this:
>>>>   void BlockFreelist::return_block(MetaWord* p, size_t word_size) {
>>>> if (word_size < SmallBlocks::small_block_min_size()) {
>>>> return; // Dark matter
>>>> }
>>>> Metablock* free_chunk = ::new (p) Metablock(word_size); if 
>>>> (word_size < SmallBlocks::small_block_max_size()) {
>>> Okay, yes, this seems like it would be a bug since 
>>> SmallBlocks::small_block_min_size() should equal sizeof Metablock in 
>>> words, but since we align up to sizeof Metablock during allocation, 
>>> we won't ever hit this.   But I'll change as suggested. 
>>
>>
>> I agree, we won't hit it.
>> But it is still better to make it right.
>> Thank you for the update!
>
> Okay, I fixed this as suggested.
>>
>>
>>
>>
>>>>   886 MetaWord* BlockFreelist::get_block(size_t word_size) {
>>>> 887 if (word_size < SmallBlocks::small_block_max_size() && 
>>>> small_blocks()->list_at(word_size).count() > 0) {
>>>>   . . .
>>>> 892 return new_block;
>>>> 893 }
>>>> 894
>>>> 895 if (word_size < BlockFreelist::min_size()) {
>>>>   896     // Dark matter.  Too small for dictionary.
>>>>   897     return NULL;
>>>>   898   }
>>>>   It'd be better to reorder the fragments 887-893 and 895-898. 
>>>> Thanks, Serguei
>>> The order is important because we first try for something in the 
>>> small_blocks array, then try for something in the dictionary.   
>>> BlockFreelist::min_size() is the size of a minimal dictionary entry, 
>>> not the same as SmallBlocks::small_block_min_size(). So the code is, 
>>> look for something in the small blocks (3-11 words) but if the size 
>>> is too small for the dictionary (12 words), return.  Then the block 
>>> is allocated from the Metachunk and not the small_blocks/dictionary 
>>> of returned blocks. Thank you for starting your review.  Please let 
>>> me know when you have further comments. Coleen 
>>
>>
>> Ok, thank you for explanation.
>> But then the additional check is needed before the L886:
>>
>>   if (word_size < SmallBlocks::small_block_min_size()) {
>>     return;
>>   }
>>
>> Otherwise, we can hit the assert in the list_at():
>>
>> 278 FreeList<Metablock>& list_at(size_t word_size) {
>> 279 assert(word_size >= _small_block_min_size, "There are no 
>> metaspace objects less than %u words", _small_block_min_size);
>> 280 return _small_lists[word_size - _small_block_min_size];
>> 281 }
>
> I also added this as suggested.

Thank you for updating the fix, Coleen!
Serguei


>
> Thanks, Serguei!
> Coleen
>> I do not have any other comments.
>> Reviewed.
>>
>>
>> Thanks,
>> Serguei
>>
>>
>>>> On 9/30/16 12:02, Coleen Phillimore wrote:
>>>>> Summary: Return Metablocks smaller than dictionary's dark matter. 
>>>>> This change contributed by Jon Masamitsu and myself.  To reclaim 
>>>>> "dark matter" this change adds an array of small blocks by size, 
>>>>> created lazily, to return Metablocks smaller than the 
>>>>> BinaryTreeDictionary entry's minimum size.   This change also 
>>>>> fixed a bug in small object double free and adds debugging code to 
>>>>> check for this case. With this change, the submitted test case 
>>>>> runs indefinitely.   Also passed rbt tier 1-5 testing. open webrev 
>>>>> at http://cr.openjdk.java.net/~coleenp/8164921.01/webrev bug link 
>>>>> https://bugs.openjdk.java.net/browse/JDK-8164921 Thanks, Coleen 
>>>>> and Jon 
>>
>