RFR(S): 8173743: Failures during class definition can lead to memory leaks in metaspace

[Volker Simonis]

On Tue, Feb 7, 2017 at 5:18 PM, Coleen Phillimore
<coleen.phillimore at oracle.com> wrote:
>
>
> On 2/7/17 9:13 AM, Volker Simonis wrote:
>>
>> Hi Mikael,
>>
>> On Tue, Feb 7, 2017 at 2:05 PM, Mikael Gerdin <mikael.gerdin at oracle.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 2017-02-06 19:40, Volker Simonis wrote:
>>>>
>>>> Hi,
>>>>
>>>> can somebody please review the following change:
>>>>
>>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8173743.v1/
>>>
>>>
>>> Reading this I come to think of the other part of this problem:
>>> failures during class byte stream parsing.
>>>
>>> Currently most of InstanceKlass::deallocate_contents is reproduced inside
>>> ~ClassFileParser - but in a slightly different way.
>>> It would be good to try to coalesce these at some point in the future
>>> otherwise I think we are going to see even more resource leaks due to the
>>> two separate implementations.
>>>
>> I totally agree. While doing this fix, I also looked at
>> InstanceKlass::deallocate_contents() and was a little surprised that
>> it seems it isn't really called anywhere until now. So yes, a cleanup
>> in 10 would be good.
>
>
> InstanceKlass::deallocate_contents is called by
> hotspot/src/share/vm/memory/metadataFactory.hpp template function
> free_metadata, and add_to_deallocate_list() is used for redefinition.  Also,

Ah, I see. It's unfortunate that Eclipse doesn't show template
functions as callers in the call hierarchy :( I also did a grep for
deallocate_contents() but missed the call from free_metadata().

Nevertheless, it seems that the only case where free_metadata() is
called on instanceKlass objects is from
ClassLoaderData::free_deallocate_list(). So adding the instanceKlass
to the deallocate list seems OK.

> inside of ClassFileParser destructor, the individual InstanceKlass fields
> are freed but once the InstanceKlass is created, it should go through
> deallocate_contents.  This would be cleaner if we had an InstanceKlass
> created first, but we can't because until we've parsed the whole thing, and
> figured out how big to make the vtables and itables, we don't know how big
> to make it.
>
> There may be more places to explicitly add an InstanceKlass to the
> deallocate list.  One instance in the bug was missed because at the time,
> the effort to make parallel class loading default was withdrawn.
>
> Thanks,
> Coleen
>
>
>>
>>> Again, I think your fix is good for 9 but I just want to share my
>>> thoughts
>>> about what we might want to fix for 10+.
>>>
>> Thanks :)
>>
>>> Thanks
>>> Mikael
>>>
>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8173743
>>>>
>>>> It fixes some problems during class definitions where instance klasses
>>>> can leak into the metaspace in cases where the class definition fails
>>>> after the class was successfully loaded from the bytecode stream.
>>>>
>>>> There are actually two cases to consider:
>>>>
>>>> 1. If we load the same class several times into the same class loader,
>>>> we will get LinkageErrors after the first successful load. But
>>>> nevertheless we will first construct a new instanceKlass for each new
>>>> load attempt and this instanceKlass will never be deleted.
>>>>
>>>> 2. If we have a parallel capable class loader, set
>>>> -XX:-UnsyncloadClass and/or -XX:+AllowParallelDefineClass and load a
>>>> class from several threads at the same time in parallel, it can happen
>>>> that we create several instance klasses for the same class. At the end
>>>> only one of them will be added to the system dictionary, but all the
>>>> other ones will never be deleted. Notice that if we run this scenario
>>>> without setting either of -XX:-UnsyncloadClass or
>>>> -XX:+AllowParallelDefineClass, this scenario will degrade into the
>>>> case above and we will get LinkageErrors for all but the first
>>>> successful load.
>>>>
>>>> The change comes with a regression test which checks for the two cases
>>>> just describe and also for the failing class redefinition case, which
>>>> currently doesn't produce a memory leak.
>>>>
>>>> I've already committed this to the hs-demo-submit/hotspot/ forest and
>>>> it went through without a problem. So in theory it should have passed
>>>> the internal JPRT tests although I'm not sure if the test set of the
>>>> "demo-submit" forest and the real hotspot repo are exactly the same
>>>> (CC'ed Tim and Brian).
>>>>
>>>> Thank you and best regards,
>>>> Volker
>>>>
>