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

[Coleen Phillimore]

Mikael,
Thank you for reviewing this code.
Coleen

On 10/11/16 3:16 AM, Mikael Gerdin wrote:
> Hi Coleen,
>
> On 2016-10-10 19:12, Coleen Phillimore wrote:
>>
>>
>>
>> On 10/10/16 4:28 AM, Mikael Gerdin wrote:
>>> Hi Coleen,
>>>
>>> On 2016-10-07 17:24, Coleen Phillimore wrote:
>>>> Mikael, thanks again ...   embedded replies and 2 questions:
>>>>
>>
>> --- cut 8< ---
>>>> 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.
>>>>
>>>> Yeah.   I didn't know if this was worth doing for 12 words or less.
>>>> Do you?
>>>
>>> To be honest: I don't know.
>>> We added the SmallBlocks free list for a reason, I guess, so maybe we
>>> should try a bit harder to conserve metaspace memory.
>>
>> The class metaspace goes through this retire_chunk code too and we
>> *don't* want to create small blocks for class metaspace.  Since I tested
>> it this way and fixes the immediate problem, I'd like to leave it as is.
>
>
> Ok.
>
>>>
>>>>
>>>>> * the remaining_words parameter which retire_current_chunk passes to
>>>>> deallocate is _exact_ and must not be aligned up through
>>>>> get_allocation_word_size.
>>>>>
>>>>
>>>> Since it's already greater than the allocation_word_size, this isn't a
>>>> problem.
>>>
>>> Well, if in a 32 bit VM a deallocation is done for an object of 13
>>> words then get_allocation_word_size will align up by 8 to 14 and the
>>> block returned could be too large.
>>> I just realized that this cannot happen since allocations are always
>>> aligned up by 8, but I wanted to share my reasoning at least.
>>>
>>
>> That's something I forget about is 32 bit aligns allocations to 8. I
>> think that's all the more reason I want retire_current_chunk() to only
>> return >12 words.
>
> Ok
>
>>>>
>>>>>
>>>>> 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.
>>>>
>>>> It is a lot of stuff I didn't want to see.  I could change it back to
>>>> trace though because the thing I really wanted to see in
>>>> MetaspaceAux::dump() is guarded by trace
>>>>
>>>>   if (log_is_enabled(Trace, gc, metaspace, freelist)) {
>>>>     if (block_freelists() != NULL) block_freelists()->print_on(out);
>>>>   }
>>>>
>>>> It seems like debug would be a better level though for both.
>>>
>>> Yeah, I think that's a good trade-off.
>>
>> Thanks, I made that change.
>
> Then I'm all done, you can add me as a Reviewer.
> Thanks
> /Mikael
>
>
>
>>
>> Coleen
>>
>>>
>>> /Mikael
>>>
>>>>>
>>>>>
>>>>>>
>>>>>> 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.
>>>>>
>>>>
>>>> thanks,
>>>> Coleen
>>>>
>>>>> /Mikael
>>>>>
>>>>>>>
>>>>>>> Coleen
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> /Mikael
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Coleen and Jon
>>>>>>>
>>>>>>
>>>>
>>