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

Daniel D. Daugherty daniel.daugherty at oracle.com
Tue Nov 27 18:34:18 PST 2012

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::grow_vs().

Dan

src/share/vm/gc_interface/collectedHeap.hpp src/share/vm/gc_interface/collectedHeap.inline.hpp Moving CollectedHeap::is_valid_method() to Method::is_valid_method() 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?

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.

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 is_valid_method() function get called? Or is my brain confusing Java "this" versus C++ "this"?

 lines 1825-1828: vtable pointer matching? The skeptic in me wonders
 how portable such a construct is across various C++ compilers.

 line 1952:   guarantee(md == NULL ||
 line 1953:       md->is_metadata(), "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"...

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

src/cpu/x86/vm/frame_x86.cpp line 537: if (!m->is_valid_method()) return false; In the post-PGR world, if this call to frame::is_interpreted_frame_valid() returns true, then the check on line 220 in forte.cpp is not necessary.

src/share/vm/prims/forte.cpp line 220: if (!method->is_valid_method()) 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.

More information about the hotspot-dev mailing list