RFR (S) 8164207: Checking missing load-acquire in relation to _pd_set in dictionary.cpp

[coleen.phillimore at oracle.com]

On 8/28/17 12:25 AM, David Holmes wrote:
> Hi Coleen,
>
> On 25/08/2017 11:26 PM, coleen.phillimore at oracle.com wrote:
>>
>> Thank you Zhengyu for noticing this change was wrong, and Christian 
>> for the idea.   New webrev:
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8164207.02/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8164207
>
> The idea of a load-acquire accessor and release_store-setter is fine 
> in principal, but it seems to me that we now use these everywhere, 
> even if we may not need them because there is no concurrent/lock-free 
> access. Overall I find it very difficult to determine what the 
> concurrent access patterns are for a Dictionary versus a 
> DictionaryEntry, and which paths are in fact lock and/or safepoint 
> free, and may be racing with locked or safepointed code. ??

That's exactly the point of making them accessors.  So one doesn't have 
to visit each individual call site and spend time answering the question 
for each case.  And probably getting it wrong.   The performance delta 
for these accesses is minimal since it's only getting the head of the 
list, not each element.

Then it's also future proof so that if a lock is removed, then we don't 
miss one of the accessors at a later time.   Note that observing bugs 
caused by this is very difficult to do, and can only be done by 
inspection.   That's why I erred on the side of safety and consistency.
>
> That aside I don't understand why you added a level of indirection 
> with the ProtectionDomainSet class?

Only the code is a level of indirection not the access.   That is to 
avoid what I said above.  See Christian's and Zhengyu's comments.
>
> Also we have been trying to include release/acquire in the names of 
> such accessors so that it is clear when we are relying on memory 
> ordering properties ie. pd_set_acquire and release_set_pd_set
>

I will change the names of these functions.

thanks,
Coleen
> Thanks,
> David
>
>
>> I reran parallel class loading tests and jck testing is in progress, 
>> but order access requires inspection.
>>
>> Thanks,
>> Coleen
>>
>>
>> On 8/24/17 5:11 PM, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 8/24/17 5:00 PM, Christian Thalinger wrote:
>>>>> On Aug 24, 2017, at 10:54 AM, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 8/24/17 4:07 PM, Zhengyu Gu wrote:
>>>>>> Hi Coleen,
>>>>>>
>>>>>> There are two instances probably overlooked?
>>>>>>
>>>>>> dictionary.cpp #103 and #124
>>>>>>
>>>>>>     for (ProtectionDomainEntry* current = _pd_set;
>>>>>> =>
>>>>>>     for (ProtectionDomainEntry* current = pd_set();
>>>>>>
>>>>>>
>>>>> Oh yeah, you're right.  That's embarrasing.   I'll fix and retest.
>>>> Which also shows that there is a potential for future mistakes. Can 
>>>> we isolate the field better so it’s only accessible via setter and 
>>>> getter?
>>>
>>> Yes, great idea.
>>> Coleen
>>>
>>>>> Thank you!!
>>>>> Coleen
>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -Zhengyu
>>>>>>
>>>>>> On 08/24/2017 02:28 PM, coleen.phillimore at oracle.com wrote:
>>>>>>> Summary: Use load_acquire for accessing DictionaryEntry::_pd_set 
>>>>>>> since it's accessed outside the SystemDictionary_lock
>>>>>>>
>>>>>>> Ran parallel class loading tests that we have as well as tier1 
>>>>>>> tests. See bug for details.
>>>>>>>
>>>>>>> open webrev at 
>>>>>>> http://cr.openjdk.java.net/~coleenp/8164207.01/webrev
>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8164207
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>
>>>
>>