RFR 8062116: JVMTI GetClassMethods is Slow

[serguei.spitsyn at oracle.com]

The fix looks good in general.

src/share/vm/oops/method.cpp

1785   bool contains(Method** m) {
1786     if (m == NULL) return false;
1787     for (JNIMethodBlockNode* b = &_head; b != NULL; b = b->_next) {
1788       if (b->_methods <= m && m < b->_methods + b->_number_of_methods) {
*1789         ptrdiff_t idx = m - b->_methods;**
**1790         if (b->_methods + idx == m) {**
1791           return true;
1792         }*
1793       }
1794     }
1795     return false;  // not found
1796   }


Just noticed that the lines 1789-1792 can be replaced with one liner:
*        return true;*

It is because the condition *(b->_methods + idx == m)* is always true.   
   :)

Also, should we check the condition: **m != _free_method*** ?
What about the following ?:
*        return (****m != _free_method***);*


Thanks,
Serguei


On 11/4/14 5:52 PM, Jeremy Manson wrote:
> Updated patch here:
>
> http://cr.openjdk.java.net/~jmanson/8062116/webrev.01/ 
> <http://cr.openjdk.java.net/%7Ejmanson/8062116/webrev.01/>
>
> Jeremy
>
> On Tue, Nov 4, 2014 at 2:15 PM, serguei.spitsyn at oracle.com 
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com 
> <mailto:serguei.spitsyn at oracle.com>> wrote:
>
>     Jeremy and Coleen,
>
>     Thank you for taking care about this bug!
>
>     The fix looks good to me.
>     I do not see any issues.
>
>     Coleen,
>
>     Please, let me know if you need any help with testing or anything
>     else.
>
>     Thanks,
>     Serguei
>
>
>     On 11/4/14 11:57 AM, serguei.spitsyn at oracle.com
>     <mailto: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.
>
>         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
>             <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.
>
>             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/
>                 <http://cr.openjdk.java.net/%7Ejmanson/8062116/webrev.00/>
>
>                 A reproducible test case can be found here:
>
>                 http://cr.openjdk.java.net/~jmanson/8062116/repro/
>                 <http://cr.openjdk.java.net/%7Ejmanson/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
>
>
>
>
>

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