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

[coleen.phillimore at oracle.com]

Here is the third webrev with the names of pd_set and set_pd_set renamed 
to pd_set_acquire and release_set_pd_set.
thanks,
Coleen

On 8/28/17 8:07 AM, coleen.phillimore at oracle.com wrote:
>
>
> 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
>>>>>>>>
>>>>
>>>
>