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

David Holmes david.holmes at oracle.com
Wed Aug 30 11:54:34 UTC 2017 

On 30/08/2017 9:14 PM, 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.
> 
> open webrev at http://cr.openjdk.java.net/~coleenp/8164207.04/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8164207

Thanks Coleen. This looks good to me.

David
-----

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

More information about the hotspot-runtime-dev mailing list