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

Coleen Phillimore coleen.phillimore at oracle.com
Wed Feb 8 14:55:48 UTC 2017

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.


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

More information about the hotspot-runtime-dev mailing list