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

[Vladimir Kozlov]

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

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

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

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