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

[Mikael Gerdin]

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