Request for review 8003635: NPG: AsynchGetCallTrace broken by Method* virtual call

[Daniel D. Daugherty]

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/view_bug.do?bug_id=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.