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

[Volker Simonis]

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.

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