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

[Mikael Gerdin]

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?

--

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.

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.

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

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
}


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

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.



Are the changes to Method and ConstMethod the "bug fix to small object 
double free"?
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?

Thanks
/Mikael



>
> Thanks,
> Coleen and Jon