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

[Coleen Phillimore]

On 2/8/17 9:47 AM, Volker Simonis wrote:
> 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.

Yes, and it's needed because the deallocate list is processed during a 
safepoint (currently) which is the only safe time to remove Klass from 
the ClassLoaderData::_klasses list.

Coleen

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