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

Coleen Phillimore coleen.phillimore at oracle.com
Wed Nov 28 07:27:57 PST 2012

Thank you Serguei! I was waiting for another review. I'm glad they are happy with this. Coleen

On 11/27/2012 5:45 PM, serguei.spitsyn at oracle.com wrote:

If you still need it, the fix looks good. Also, the Solaris studio guys are pretty happy with this fix. :)

Thanks, Serguei On 11/21/12 9:13 AM, Coleen Phillimore wrote:

Thanks again for the code review, David. On 11/20/2012 10:26 PM, David Holmes wrote: Hi Coleen,

On 21/11/2012 8:24 AM, Coleen Phillimore wrote: Summary: Make metaspace::contains be lock free and used to see if I don't think this is 100% valid without assuming TSO. Your are growing a linked list of nodes under a lock, but allowing the existing list to be iterated without a lock. You have to ensure that a VirtualSpaceNode can't be seen in the list prior to being properly initialized - I know the code in VirtualSpaceNode::initialize makes it unlikely, but I wouldn't want to second-guess how the compiler and/or hardware might reorder things. To be safe I think you need: 980 // Allocate the meta virtual space and initialize it. 981 VirtualSpaceNode* newentry = new VirtualSpaceNode(vsbytesize); 982 if (!newentry->initialize()) { 983 delete newentry; 984 return false; 985 } else { + // ensure lock-free iteration sees fully initialized node + OrderAccess:storeStore(); 986 linkvs(newentry, vswordsize); 987 return true; 988 } Thank you! I added the OrderAccess call. I feel better about the safety of walking this lock free now.

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 Do the comments in forte.cpp regarding the unsafe reference to the method not still apply? There are better comments above this comment. The interpreter frame could be partially constructed when AsyncGetCallTrace picks it up. We no longer have to worry about GC making the Method* invalid (except for bugid 8003720 <https://jbs.oracle.com/bugs/browse/JDK-8003720>). The second check is probably not strictly necessary since that was what it was checking, but for safety I want to leave it in. Thanks, Coleen Cheers, David ----- Tested with full NSK testlist on linux 32. Studio tools group also tested this. Thanks, Coleen

More information about the hotspot-dev mailing list