RFR 8062116: JVMTI GetClassMethods is Slow

[serguei.spitsyn at oracle.com]

On 11/4/14 12:43 PM, Coleen Phillimore wrote:
>
> On 11/04/2014 02:57 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Jeremy and Coleen,
>>
>> I'm reviewing this too.
>> We also need to run the nsk.jvmti, nsk.jdi and jtreg jdi tests.
>
> Hi Serguei,  I ran all of vm.quick.testlist on this which includes 
> jvmti, jdi tests.  I'll run jtreg jdi tests too (where are they?)

Hi Coleen,

It is more safe to run the nsk.jvmti.testlist and nsk.jdi.testlist 
instead of the vm.quick.testlist.
The jtreg jdi tests are in the <repo>/jdk/test/com/sun/jdi folder.

Thanks,
Serguei

>
> Thanks,
> Coleen
>
>>
>> Thanks,
>> Serguei
>>
>> On 11/3/14 12:19 PM, Coleen Phillimore wrote:
>>>
>>> Hi Jeremy,
>>>
>>> I reviewed your new code and it looks fine.  I had one comment in
>>>
>>> http://cr.openjdk.java.net/~jmanson/8062116/webrev.00/src/share/vm/prims/jvmtiEnv.cpp.udiff.html 
>>>
>>>
>>> The name "need_to_resolve" doesn't make sense when reading this 
>>> code.  Isn't it more like "need_to_ensure_space" ?  I think method 
>>> resolution with the other name, which it doesn't do.
>>>
>>> I was trying to find a way to make this new code not appear twice 
>>> (maybe with a local jvmtiEnv function get_jmethodID(m) - instanceK_h 
>>> is m->method_holder()).
>>
>> Agreed on the above.
>>
>>>
>>> Also, I was trying to figure out if the new class in utilities 
>>> called chunkedList.hpp could be used to store jmethodIDs, since the 
>>> data structures are similar.  There is still more things in 
>>> JNIMethodBlock has to do so I think a specialized structure is still 
>>> needed (which is why I originally wrote it to be very simple).  I'm 
>>> not sure if the comment above it still applies. Maybe only the first 
>>> and third sentences.  Can you rewrite the comment slightly?
>>>
>>> Your other comments in the changes are good.
>>>
>>> I can't completely answer your question about reusing free_methods - 
>>> but if a jmethodID is created provisionally in 
>>> InstanceKlass::get_jmethod_id and not needed because it loses the 
>>> race in the method id cache, it's never handed back to native code, 
>>> so it's safe to reuse.  This is different than jmethodIDs for 
>>> methods that are unloaded.  They are cleared and never reused.  At 
>>> least that's my reading of this caching code but it's pretty 
>>> complicated stuff.
>>>
>>> I've also run our nsk and jck vm/jvmti on this change and they all 
>>> passed.  I'd be happy to sponsor it with these suggested changes and 
>>> it needs another reviewer.
>>>
>>> Thanks for diagnosing and fixing this problem!
>>> Coleen
>>>
>>>
>>> On 10/30/2014 01:02 PM, Jeremy Manson wrote:
>>>> There's a significant regression in the speed of JVMTI 
>>>> GetClassMethods in
>>>> JDK8. I've tracked this down to allocation of jmethodids in a tight 
>>>> loop.
>>>> The issue can be addressed by preallocating enough space for all of 
>>>> the
>>>> jmethodids when starting the operation and not iterating over all 
>>>> of the
>>>> existing jmethodids when you allocate a new one.
>>>>
>>>> A patch is here:
>>>>
>>>> http://cr.openjdk.java.net/~jmanson/8062116/webrev.00/
>>>>
>>>> A reproducible test case can be found here:
>>>>
>>>> http://cr.openjdk.java.net/~jmanson/8062116/repro/
>>>>
>>>> It's a benchmark, though: I have no idea how to turn it into a test.
>>>>
>>>> For whoever reviews it: can you explain to me why it is okay that 
>>>> this code
>>>> reuses jmethodIDs (in JNIMethodBlock::add_method?  I can imagine a 
>>>> lot of
>>>> problems stemming from accidental reuse.
>>>>
>>>> Jeremy
>>>
>>
>