RFR 8062116: JVMTI GetClassMethods is Slow

[Coleen Phillimore]

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

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