On 11/4/14 12:43 PM, Coleen Phillimore wrote:
On 11/04/2014 02:57 PM, 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.
Hi Serguei, I ran all of vm.quick.testlist on this which includes
jvmti, jdi tests. I'll run jtreg jdi tests too (where are they?)
Hi Coleen,
It is more safe to run the nsk.jvmti.testlist and nsk.jdi.testlist
instead of the vm.quick.testlist.
The jtreg jdi tests are in the <repo>/jdk/test/com/sun/jdi folder.
Thanks,
Serguei
Thanks,
Coleen
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
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/
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
