RFR 8062116: JVMTI GetClassMethods is Slow

[Coleen Phillimore]

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

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