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

[serguei.spitsyn at oracle.com]

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