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

[David Holmes]

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 aside I don't understand why you added a level of indirection with 
the ProtectionDomainSet class?

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

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