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

Wed Aug 30 11:14:40 UTC 2017

Hi, I changed the edit for David to only use ordering semantics in the 
places where needed in the lock free access to pd_set.  Since only 
contains_protection_domain is read lock free, it should be ok.

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

Thanks,
Coleen

On 8/29/17 2:28 AM, David Holmes wrote:
> Hi Coleen,
>
> On 29/08/2017 5:39 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> 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
>
> This API should also be renamed:
>
> !   ProtectionDomainEntry* pd_set() const            { return 
> _inner.pd_set_acquire(); }
> !   void set_pd_set(ProtectionDomainEntry* new_head) { 
> _inner.release_set_pd_set(new_head); }
>
> These are the ones that need to give visibility to the fact we're 
> accessing things lock-free (if indeed we are).
>
> More below ...
>
>>> 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.
>
> Sorry, it may sound strange to say that I don't agree with "erring on 
> the side of safety and consistency" but I do not agree with just using 
> acquire/release semantics everywhere just in case! If we don't know 
> the lock-free paths then how can we possibly know things are correct. 
> The whole point of these accessors is to make it obvious where the 
> lock-free accesses are.
>
>>>>>
>>>>> 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.
>
> Okay - I see what you did but I would not expect to have to protect 
> _pd_set from direct use within its own class - anyone messing with 
> that class should be aware of the need to use the accessors. Though I 
> suppose this encapsulation is little different to defining the field 
> as some kind of "Atomic" type rather than a "raw" type.
>
> Thanks,
> David
> -----
>
>>>>>
>>>>> 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
>>>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>