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

[Coleen Phillimore]

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

"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 is_valid_method()
>         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->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"...

I think we have a lot of these left over.

>
> 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.

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/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.

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

Thanks!
Coleen