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

[coleen.phillimore at oracle.com]

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?

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.

You should add this new type at
http://cr.openjdk.java.net/~poonam/8199406/webrev.01/src/hotspot/share/code/codeBlob.hpp.html

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