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

[Stefan Karlsson]

On 2013-12-10 12:26, Stefan Johansson wrote:
> 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.

Looks good to me.

StefanK

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