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.phillim...@oracle.com <mailto:coleen.phillim...@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
