Request for review 8003635: NPG: AsynchGetCallTrace broken by Method* virtual call (original) (raw)

Coleen Phillimore coleen.phillimore at oracle.com
Wed Nov 28 09:02:45 PST 2012

Thank you for doing this review, Dan. In response to one of your comments below, I made a change to globalDefinitions.hpp to add dereference_vptr() to get the vptr for an object, so that the assumption in a couple of places is in a file that can refactor for compiler differences if they exist.

http://cr.openjdk.java.net/~coleenp/8003635_2/

On 11/27/2012 9:34 PM, Daniel D. Daugherty wrote:

On 11/20/12 3:24 PM, Coleen Phillimore wrote:

Summary: Make metaspace::contains be lock free and used to see if something is in metaspace, also compare Method* with vtbl pointer.

open webrev at http://cr.openjdk.java.net/~coleenp/8003635/ bug link at http://bugs.sun.com/viewbug.do?bugid=8003635 Tested with full NSK testlist on linux 32. Studio tools group also tested this. Thanks, Coleen Thumbs up! And a very nice catch by David H in VirtualSpaceList::growvs(). Dan src/share/vm/gcinterface/collectedHeap.hpp src/share/vm/gcinterface/collectedHeap.inline.hpp Moving CollectedHeap::isvalidmethod() to Method::isvalidmethod() makes sense since methods aren't in the heap anymore. src/share/vm/memory/allocation.cpp No comments. src/share/vm/memory/metaspace.hpp Was: bool contains(const void *ptr) const; and is now: static bool contains(const void *ptr); So why was the trailing "const" dropped?

"const" applies to the "this" pointer and making it static means there isn't a "this" pointer.

src/share/vm/memory/metaspace.cpp No comments except I don't see the changes that David Holmes requested so I'm going to guess that you didn't refresh the webrev in place. I'm OK if you didn't.

I haven't replaced the webrev.

src/share/vm/oops/method.hpp No comments. src/share/vm/oops/method.cpp line 1820: if (this == NULL) { If "this" is NULL, then how did this non-static isvalidmethod() function get called? Or is my brain confusing Java "this" versus C++ "this"?

David answered this "this" question.

lines 1825-1828: vtable pointer matching? The skeptic in me wonders how portable such a construct is across various C++ compilers. It's not portable except every C++ compiler puts the main vtbl pointer as the first word in a class. I'll add a comment though. This assumption is also made in universe.cpp for CDS vtbl pointer patching.

I just added a deference_vptr in globalDefinitions.hpp with a comment about it's portability. Will post another webrev.

line 1952: guarantee(md == NULL || line 1953: md->ismetadata(), "should be in permspace"); Not part of your change, but this mention of "permspace" caught my eye. I'm not caught on all the post-PGR terms, but is this the right word to use? The guarantee() on line 1950 says "should be metadata"...

I think we have a lot of these left over.

src/cpu/sparc/vm/framesparc.cpp line 651: if (!m->isvalidmethod()) return false; In the post-PGR world, if this call to frame::isinterpretedframevalid() returns true, then the check on line 220 in forte.cpp is not necessary.

Yes, I realized this but was afraid to change it. I'm still wary of changing more since I wasn't able to test this properly myself.

src/cpu/x86/vm/framex86.cpp line 537: if (!m->isvalidmethod()) return false; In the post-PGR world, if this call to frame::isinterpretedframevalid() returns true, then the check on line 220 in forte.cpp is not necessary. src/share/vm/prims/forte.cpp line 220: if (!method->isvalidmethod()) return false; Methods can no longer move in the post-PGR world, right? If that's the case, then this check isn't needed since the frame was validated on line 199. However, the extra paranoia doesn't hurt.

I completely agree. The extra paranoia doesn't really hurt anything here.

Thanks! Coleen

More information about the hotspot-dev mailing list