RFR 8062116: JVMTI GetClassMethods is Slow

[Coleen Phillimore]

Hi Jeremy,

Having Chuck reply publicly to the review would be good.  We miss seeing 
his emails :)

On 11/04/2014 02:58 PM, Jeremy Manson wrote:
>
> Thanks for taking a look, Coleen!
>
> On Mon, Nov 3, 2014 at 12:19 PM, Coleen Phillimore 
> <coleen.phillimore at oracle.com <mailto: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
>     <http://cr.openjdk.java.net/%7Ejmanson/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)?

Okay, yes jmethodids_found makes more sense to me in this context.

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

Oh, yes.  You are right.  The code is fine then.  It's not too much 
duplicated.

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

No, I don't think you should do this.  It was a general comment that 
this utility class is available for such things but has only one use so far.

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

Thanks!

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

Thank you!   If you commit and send me the result of hg export your 
changeset, then I'll get your comments also and won't get the chance to 
mess up and not use commit -u jmanson.

Coleen

> Jeremy
>

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