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