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

[Coleen Phillimore]

Hi Mikael,

Thanks for looking at this change.

On 10/4/16 8:32 AM, Mikael Gerdin wrote:
> Hi Coleen,
>
> On 2016-09-30 21: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
>
> I'd prefer it if SmallBlocks didn't expose its implementation by 
> returning its FreeLists by reference, could you change it to have
> * return_chunk()
> * get_chunk()
> * num_chunks(word_size)
>
> and get rid of list_at?

Okay, I refactored this so small_chunks()->get_block() and 
return_block() are used rather than getting list_at.  I didn't see where 
you got num_chunks, but list_at is hidden.
>
> -- 
>
> For how long do you plan to keep BlockFreelist::_all_blocks?
> I see that it's debug only but I fear that it could case problems with 
> running out of native memory in our internal testing of debug builds.

I thought about taking it out, but it helped me find the double free 
bug.   I think if we add new code to metadata and have to call 
deallocate_contents on it, we risk re-introducting these double free 
bugs.   I could take it out.  I don't think this gets that big but I'd 
hate to introduce some sort of OOM bug in our testing.
>
> BlockFreelist::min_size() is a bit of a misnomer since it returns the 
> minimum size of blocks to be put on the BlockTreeDictionary, not the 
> minimum size of blocks which are reusable.

How about min_dictionary_size() ?

>
> Is there any particular reason behind _small_blocks being lazy 
> allocated and _dictionary not?

We lazily create the BlockFreelists with this change.

   // Lazily create a block_freelist
   if (block_freelists() == NULL) {
     _block_freelists = new BlockFreelist();
   }

So the small_blocks are lazily created in the BlockFreelists but the 
dictionary is not.  I guess if we're going to create the BlockFreelists 
here, we'll most likely need both and maybe small_blocks need not be 
lazily created.  Was that your suggestion?

My concern with this change was all the space used by the small_blocks() 
but if we're doing any deallocation within the metaspace, at least one 
of the things will be <12 words.

I'll make small_blocks() not be lazily allocated since BlockFreelist 
are.  These are pretty expensive, but should be limited to a few metaspaces.
>
> I would prefer if BlockFreelist::return_block would perform the checks 
> in reverse order instead of having a return inside the first if block, 
> something like
>
> if (word_size > small_block_max_size) {
> dict()->return_chunk(c)
> } else if (word_size > small_block_min_size) {
> small_blocks()->return_chunk(c)
> } else {
>   // dark matter
> }

Why?  We don't want to cast Metablock into dark matter so check if 
word_size < small_block_min_size first.

   Metablock* free_chunk = ::new (p) Metablock(word_size);
   if (word_size < SmallBlocks::small_block_max_size()) {
     small_blocks()->return_chunk(word_size);
   } else {
     dictionary()->return_chunk(free_chunk);
   }

>
>
> For BlockFreelist::get_block I realize that the code is a bit more 
> complex but this also raises a few questions.
> * When we allocate from the dictionary we search for a block which is 
> at least as large as we ask for and split it, returning whatever was 
> left back to the free list. We don't search for "large enough" blocks 
> from the small blocks manager, is that intentional or just to keep the 
> code simple (I like simple)?

I'm glad you asked about this so I could give background.   It turns out 
that we deallocate metaspace items better this way.  I had a version 
that did exactly what you said.  It was a simple sorted linked list of 
returned blocks < min_dictionary_size (12) where get_block returned the 
first block where the item would fit.  It had some best fit algorithm so 
if the block returned was a lot bigger, it wouldn't pick it.

My implementation could get through 69300 retransformations before the 
list didn't work anymore (too many small block fragments of the wrong 
size) and metaspace was exhausted (metaspace was limited to 12M in this 
test).  Jon's implementation ran this test indefinitely.  So it's 
somewhat simple but it worked really well.

>
> In the last part of get_block where we return the unused part of a 
> block retrieved from the dictionary uses compares with 
> BlockFreelist::min_size() which, as I mentioned above, is the min size 
> for blocks in the dictionary, not the min size for blocks to be reusable.
> I think this code can just call return_block unconditionally for any 
> nonzero value of "unused" and let return_block deal with dark matter.
>
yes, I'll make that change.
>
>
> Are the changes to Method and ConstMethod the "bug fix to small object 
> double free"?

Yes.
> Is the problem that when a method was freed its annotations were 
> deallocated as well? Could the annotations fields in the "old" 
> ConstMethod be cleared instead so that the old annotation arrays could 
> be kept or is that just needless complexity?

I'd rather copy the annotations, because I don't know how the old 
method, which could still be running, might use the annotations.  I 
don't want to mess with that but I see your point.

Coleen

>
> Thanks
> /Mikael
>
>
>
>>
>> Thanks,
>> Coleen and Jon