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.spit...@oracle.com
<mailto:serguei.spit...@oracle.com> <serguei.spit...@oracle.com
<mailto:serguei.spit...@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.spit...@oracle.com
<mailto:serguei.spit...@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
