RFR 8062116: JVMTI GetClassMethods is Slow

[serguei.spitsyn at oracle.com]

Hi Jeremy and Coleen,

I'm reviewing this too.
We also need to run the nsk.jvmti, nsk.jdi and jtreg jdi tests.

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
>