RFR 8062116: JVMTI GetClassMethods is Slow

[Jeremy Manson]

Thanks for taking a look, Coleen!

On Mon, Nov 3, 2014 at 12:19 PM, Coleen Phillimore <
coleen.phillimore at oracle.com> 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.
>

Hmmm... it is there to tell you that there are jmethodids for that class
that haven't been instantiated.  Is it all right if I change it to
"jmethodids_found" (and reverse the sense of it)?


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

You know, I initially did that, but this file is parsed with some weird XSL
setup that doesn't allow methods other than the ones that map directly to
the JVMTI calls.

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

chunkedList wouldn't work as is, because it doesn't let you parameterize
the bucket size, but it could probably be made to work (in the same way I
made this one work).  It's also an oddly bare-bones class - I'm not sure
why it doesn't have contains and insert methods and so on.

I'm not in love with the idea of doing it, because a) it would complicate
my backport and b) I don't really have a lot of time to do hotspot
refactoring, but if you think it should happen, I can make it happen
(perhaps not in a timely way :) ).

As for the comment, I'll eliminate all but the first and third sentences.


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

Ah, I see.  Thanks.


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

I've cc'd Chuck Rasbold, who has already reviewed it internally and given
it the thumbs-up.  I'm sure he would be happy to do so publicly, too.

Thanks for diagnosing and fixing this problem!


Happy to do it!  And so are the programs that use my JVMTI!

Jeremy
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20141104/1b147eeb/attachment.html>