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

Wed Aug 30 23:00:44 UTC 2017

> On Aug 30, 2017, at 1:14 AM, coleen.phillimore at oracle.com wrote:
> 
> 
> 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.

Sorry, I lost track.  Are there now raw accesses to _pd_set?  And if yes, does a comment say why?

> 
> open webrev at http://cr.openjdk.java.net/~coleenp/8164207.04/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8164207 <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
>>>>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>> 
>>>> 
>>> 
> 