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

[Coleen Phillimore]

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