RFR (M) 8164921: Memory leaked when instrumentation.retransformClasses() is called repeatedly
Mikael Gerdin
mikael.gerdin at oracle.com
Fri Oct 7 14:39:22 UTC 2016
Hi Coleen,
On 2016-10-06 19:54, Coleen Phillimore wrote:
>
> Here is an update to the Metadata leak change. There was a bug
> introduced when cleaning this up, which Mikael also found.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8164921.02/webrev
>
> Changes include adding logging for report_metadata_oome, which
> necessitated removing ResourceMarks in ClassLoaderData::dump because the
> stream passed in already required a ResourceMark, so it got a nested
> ResourceMark message for the stream.
>
> I changed logging for tracing block allocations to log_trace(gc,
> metaspace, freelist, blocks).
>
> In BlockFreelist::get_block and BlockFreelist::return_block() assert
> that it's not called for a size smaller than the minimum allocation
> (which was the bug). Renamed get_raw_word_size() to
> get_allocation_word_size(). This rounds up to the minimum allocation
> size which is the same as small_block_min_size.
>
> Also, I added a test that takes a long time to execute to verify this,
> and excluded it from JPRT. I could skip adding this test if people
> don't want it. Also, the test verifies that continuously redefining the
> same class gets memory for the new class that was released because the
> block sizes are the same. When the test exits, it gets a metaspace OOM
> because loading new classes and allocating metadata can't use the blocks
> returned (wrong size). There is still fragmentation in this
> implementation, but it's better that things deallocated < 12 words are
> actually freed. I'll file an RFE to work on a perfect algorithm, or to
> investigate finding a better one, although I consider this a stress test
> that uses all of metaspace to MaxMetaspaceSize, leaving allocation only
> to the block fragments left. This isn't a typical use case.
>
> Some comments and corrections to my responses to Mikael below:
>
> On 10/4/16 12:15 PM, Coleen Phillimore wrote:
>>
>> 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.
Thanks, that's much better!
>>>
>>> --
>>>
>>> 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.
Perhaps a trade off could be to have a bound on the size of the array
and deallocate and disable it if it grows too large?
>>>
>>> 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() ?
That's fine.
>>
>>>
>>> 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.
>
> Class SpaceManager doesn't need small_blocks() so I left small_blocks as
> lazily allocated.
Ok, I agree that the size of small_blocks() motivates its lazy
allocation scheme.
>>>
>>> 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);
>> }
>>
>
> There is no dark matter in these functions anymore.
Oh! Right.
>>>
>>>
>>> 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.
>
> This change has to be conditional because I assert that
> BlockFreelist::return_block() is never called for < small_block_min_size.
Ok.
There's one more thing, though.
In SpaceManager::retire_current_chunk the last piece of the current
allocation chunk is made reusable through a call to
SpaceManager::deallocate. There are two problems with this:
* retire_current_chunk only calls deallocate for wasted blocks larger
than the min dictionary size, this means that wasted blocks which would
fit in SmallBlocks will be wasted instead of reusable.
* the remaining_words parameter which retire_current_chunk passes to
deallocate is _exact_ and must not be aligned up through
get_allocation_word_size.
Also,
are you sure about logging the output of ClassLoaderData::dump on the
info level? Perhaps logging the cld dump on the debug level is a good
trade-off? It looks like the trace level causes the VSM to print the
entire free lists to the log so that's probably a bit much.
>
> Thanks,
> Coleen
>
>>>
>>>
>>> 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.
That's fine I think, I wouldn't want to mess around with it either. I
just wanted to understand the reasoning behind the change, thanks for
confirming my suspicion.
/Mikael
>>
>> Coleen
>>
>>>
>>> Thanks
>>> /Mikael
>>>
>>>
>>>
>>>>
>>>> Thanks,
>>>> Coleen and Jon
>>
>
More information about the hotspot-dev
mailing list