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

[Poonam Parhar]

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; }
>
>>
>> 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.
>
Sure, I will add the new type to the comments.

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