RFR: JDK-8199406: Performance drop with Java JDK 1.8.0_162-b32

[Vladimir Kozlov]

On 3/30/18 10:31 AM, coleen.phillimore at oracle.com wrote:
> 
> 
> On 3/30/18 12:43 PM, Poonam Parhar wrote:
>> Thanks for looking at the changes, Coleen!
>>
>> On 3/30/2018 9:25 AM, Vladimir Kozlov wrote:
>>> On 3/30/18 8:31 AM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> http://cr.openjdk.java.net/~poonam/8199406/webrev.01/src/hotspot/share/code/vtableStubs.cpp.udiff.html 
>>>>
>>>>
>>>>     uint hash = VtableStubs::hash(stub->is_vtable_stub(), 
>>>> stub->index());
>>>>
>>>>
>>>> Isn't stub->is_vtable_stub() always true here, so you could avoid 
>>>> another virtual call?  Can you assert this and pass true here?
>>>
>>> Not true. It could be also itable. They differentiate by value of 
>>> _is_vtable_stub field. It is not virtual call, it is check of the field:
>>>
>>> bool is_vtable_stub() { return  _is_vtable_stub; }
> 
> Thank you for the clarification.  I had it mixed up with 
> is_vtable_blob().  Glad it's not virtual.
>>>
>>>>
>>>> The whole subclassing of BufferBlob with an virtual call 
>>>> is_vtable_blob() seems inefficient if you have to call this all the 
>>>> time during unloading and otherwise, and spending a word in one of 
>>>> the alignment gaps in CodeBlob to point to the type of code blob 
>>>> would be more efficient.  But that might be an RFE for the compiler 
>>>> team.  Also since the only virtual functions are "is_<which derived 
>>>> type>" functions.
>>>
>>> I think we discussed that during rewriting this code for AOT. I don't 
>>> remember why we decided to keep it this way. May be because it did 
>>> not show up on our code performance profiling. On x86 it is really fast.
>>>
>>> May be we should review this code again if you think it should be 
>>> optimized.
>>>
> 
> It might be worth discussing, if this code continues to matter for 
> unloading method performance.

Devirtualizing does not always help unless it is in very critical code 
but it may complicate things for partially constructed subclass objects 
(virtual pointer is initialized first but we don't know when fields will 
be initialized).

The first thing which was suggested and Poonam implemented was 
devirtualize CompiledICHolder(). But it did not help at all.

Vladimir

> 
> 
>>>>
>>>> You should add this new type at
>>>> http://cr.openjdk.java.net/~poonam/8199406/webrev.01/src/hotspot/share/code/codeBlob.hpp.html 
>>>
>>>
>>>
>>>
>>> I thought it was there in one of the working versions. Yes, it needs 
>>> to be added to that comment.
>>>
>> Sure, I will add the new type to the comments.
> 
> Thanks!  Thank you for fixing this issue!
> Coleen
>>
>> Thanks,
>> Poonam
>>
>>> Thanks,
>>> Vladimir
>>>
>>>>
>>>>
>>>> line 61.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 3/29/18 6:14 PM, Poonam Parhar wrote:
>>>>> Hello Vladimir,
>>>>>
>>>>> Thanks for reviewing the changes. Please find the updated webrev here:
>>>>> http://cr.openjdk.java.net/~poonam/8199406/webrev.01/
>>>>>
>>>>> Regards,
>>>>> Poonam
>>>>>
>>>>> On 3/29/2018 2:23 PM, Vladimir Kozlov wrote:
>>>>>> Looks good. I have few comments.
>>>>>>
>>>>>> You can remove old is_entry_point() which have the same code as 
>>>>>> new entry_point(). After changes it is used only in one place in 
>>>>>> CompiledIC::is_megamorphic(). It can be replace with NULL check of 
>>>>>> entry_point() result there. Add comment in vtableStubs.hpp for new 
>>>>>> method.
>>>>>>
>>>>>> is_metadata_method field name should start with _ (this is Hotspot 
>>>>>> convention).
>>>>>>
>>>>>> is_loader_alive() could be simplified since you have field now:
>>>>>>
>>>>>>   inline bool is_loader_alive(BoolObjectClosure* is_alive) {
>>>>>>     Klass* k = is_metadata_method ? 
>>>>>> ((Method*)_holder_metadata)->method_holder() : 
>>>>>> (Klass*)_holder_metadata;
>>>>>>     if (!k->is_loader_alive(is_alive)) {
>>>>>>       return false;
>>>>>>     }
>>>>>>     if (!_holder_klass->is_loader_alive(is_alive)) {
>>>>>>       return false;
>>>>>>     }
>>>>>>     return true;
>>>>>>   }
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 3/29/18 1:31 PM, Poonam Parhar wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review the changes for the following bug that improve the 
>>>>>>> nmethod unloading times with a couple of optimizations.
>>>>>>>
>>>>>>> JDK-8199406 <https://bugs.openjdk.java.net/browse/JDK-8199406>: 
>>>>>>> Performance drop with Java JDK 1.8.0_162-b32
>>>>>>> Webrev: http://cr.openjdk.java.net/~poonam/8199406/webrev.00/
>>>>>>>
>>>>>>> This changeset includes two changes:
>>>>>>> 1. In /compiledIC.cpp, CompiledIC::is_icholder_entry()/ , we need 
>>>>>>> to determine if the code blob is an itable stub. With this 
>>>>>>> change, before linearly searching through all the VtableStub 
>>>>>>> entries, we first check whether the codeblob is a vtable or not. 
>>>>>>> We now also parse through the list entries only once rather than 
>>>>>>> doing it twice in /VtableStubs::is_entry_point()/ and 
>>>>>>> /VtableStubs::stub_containing()/.
>>>>>>> 2. The second change helps avoid the virtual function calls in 
>>>>>>> CompiledICHolder::is_loader_alive(). CompiledICHolder now stores 
>>>>>>> information whether the metadata it holds is a method or a klass.
>>>>>>>
>>>>>>> Testing:
>>>>>>> - Customer testing confirming that their class-unloading times 
>>>>>>> drop from 10s of seconds to an average of 0.75 secs.
>>>>>>> - mach5 jdk-tier1,jdk-tier2,jdk-tier3,hs-tier1,hs-tier2
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Poonam
>>>>>>>
>>>>>
>>>>
>>
>