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

[David Holmes]

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