RFR: 8028993: Full collections with ParallelScavenge slower in JDK 8 compared to 7u40

[Stefan Johansson]

On 2013-12-10 10:20, Stefan Johansson wrote:
> Hi Karlsson =)
>
> On 2013-12-09 13:15, Stefan Karlsson wrote:
>> Hi Johansson,
>>
>> On 2013-12-09 10:33, Stefan Johansson wrote:
>>> Hi all,
>>>
>>> Updated one thing in the webrev after an offline comment. The new 
>>> webrev is at:
>>> http://cr.openjdk.java.net/~sjohanss/8028993/webrev.01/
>>
>> I think this looks good.
>>
>> A comment about this comment.
>>
>> http://cr.openjdk.java.net/~sjohanss/8028993/webrev.01/src/share/vm/oops/instanceMirrorKlass.cpp.udiff.html 
>>
>>
>> +    // For anonymous classes we need to handle the class loader data,
>> +    // otherwise it won't be claimed and can be unloaded.
>> +    if (klass->oop_is_instance() && 
>> InstanceKlass::cast(klass)->is_anonymous()) {
>> +      PSParallelCompact::follow_class_loader(cm, 
>> klass->class_loader_data());
>> +    } else {
>>      PSParallelCompact::follow_klass(cm, klass);
>> +    }
>>
>>
>> IMHO, the comment doesn't help with the understanding of this code. 
>> All klasses, not only anonymous klasses, "need to handle the class 
>> loader data, otherwise it won't be claimed and can be unloaded."
>>
>> Could you explain that:     // Anonymous classes doesn't have their 
>> own class loader and the
>>     // the call to follow_klass will mark and push its java mirror
>>     // instead of the class loader. When handling the java mirror for
>>     // an anonymous class we need to make sure its class loader data is
>>     // claimed, this is done by calling follow_class_loader explicitly,
>>     // for non-anonymous classes the call to follow_class_loader is made
>>     // when the class loader itself is handled.
>> 1) We can't call follow_klass, since that would just try to follow 
>> the mirror we're currently processing?
>> 2) That for non-anonymous klasses we call follow_class_loader when 
>> the java.lang.ClassLoader of the klass is visited. But for anonymous 
>> klasses, which don't have their own java.lang.ClassLoader instance, 
>> we instead call follow_class_loader when the mirror is visited?
>>
> We've spoken about this offline and this is the comment I plan to add, 
> I will also change the comment in the SerialGC version of the code:
>     // Anonymous classes doesn't have their own class loader and the
>     // the call to follow_klass will mark and push its java mirror
>     // instead of the class loader. When handling the java mirror for
>     // an anonymous class we need to make sure its class loader data is
>     // claimed, this is done by calling follow_class_loader explicitly,
>     // for non-anonymous classes the call to follow_class_loader is made
>     // when the class loader itself is handled.
Got some input on the comment and here is the (hopefully) final version:
     // An anonymous class doesn't have its own class loader, so the call
     // to follow_klass will mark and push its java mirror instead of the
     // class loader. When handling the java mirror for an anonymous class
     // we need to make sure its class loader data is claimed, this is done
     // by calling follow_class_loader explicitly. For non-anonymous classes
     // the call to follow_class_loader is made when the class loader itself
     // is handled.

StefanJ
>
> Cheers,
> StefanJ
>> thanks,
>> StefanK
>>
>>>
>>> The only change was to move the definition of follow_klass() in 
>>> psParallelCompact.hpp to be below the definition of mark_and_push(). 
>>> This might help inlining for some compilers, and it also maps to how 
>>> the declarations are ordered.
>>>
>>> Please review this fix,
>>> Stefan
>>>
>>>
>>> On 2013-12-04 17:48, Stefan Johansson wrote:
>>>> Hi,
>>>>
>>>> Can I have some reviews for this change to fix:
>>>> https://bugs.openjdk.java.net/browse/JDK-8028993
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sjohanss/8028993/webrev.00/
>>>>
>>>> Summary:
>>>> While fixing JDK-8027675 we realized that we should have a similar 
>>>> regression with ParallelGC. The regression is not as big as with 
>>>> serial but the logic changes are the same and I've been able to 
>>>> measure a significant improvement with this fix, taking us on par 
>>>> with 7u40.
>>>>
>>>> The theory behind the fix is the same as when fixing serial:
>>>> ---
>>>> To improve the mark phase:
>>>> * Enabled the calls to follow_klass to be inlined.
>>>> * Reduce the extensive amount of calls to follow_class_loader in 
>>>> follow_klass and instead just mark the klass holder (the class 
>>>> loader or the java mirror) for the given klass.
>>>>
>>>> To improve the adjust phase:
>>>> * All calls to adjust_klass have been removed since this is already 
>>>> handled by processing the ClassLoaderDataGraph.
>>>> ---
>>>>
>>>> Testing:
>>>> * Building and basic testing through JPRT
>>>> * Some manual testing running Nashorn tests
>>>> * Performance testing with SPECjbb2005
>>>>
>>>> Thanks,
>>>> Stefan
>>>
>>
>