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

[David Holmes]

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* new_entry = new VirtualSpaceNode(vs_byte_size);
  982   if (!new_entry->initialize()) {
  983     delete new_entry;
  984     return false;
  985   } else {
+        // ensure lock-free iteration sees fully initialized node
+        OrderAccess:storeStore();
  986     link_vs(new_entry, vs_word_size);
  987     return true;
  988   }

> 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

Do the comments in forte.cpp regarding the unsafe reference to the 
method not still apply?

Cheers,
David
-----

> Tested with full NSK testlist on linux 32. Studio tools group also
> tested this.
>
> Thanks,
> Coleen