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

[coleen.phillimore at oracle.com]

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.


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