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

[coleen.phillimore at oracle.com]

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

open webrev at http://cr.openjdk.java.net/~coleenp/8164207.03/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8164207

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