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

[Coleen Phillimore]

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

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