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

[serguei.spitsyn at oracle.com]

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!




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