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

[Coleen Phillimore]

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* 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   }

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