RFR (M) 8186088: ConstantPoolCache::_resolved_references is not a JNIHandle

[coleen.phillimore at oracle.com]

On 8/14/17 6:27 PM, coleen.phillimore at oracle.com wrote:
>
>
> On 8/14/17 4:36 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 15/08/2017 1:38 AM, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 8/13/17 9:04 PM, David Holmes wrote:
>>>> Hi Coleen,
>>>>
>>>> On 12/08/2017 7:34 AM, coleen.phillimore at oracle.com wrote:
>>>>> Summary: Make an OopHandle type to replace jobject to encapsulate 
>>>>> these oop pointers in metadata and module entry.
>>>>>
>>>>> This replaces places where there's a jobject that doesn't point 
>>>>> into the JNIHandles, notably things allocated in 
>>>>> ClassLoaderData::_handles.
>>>>>
>>>>> There were platform specific changes that I tried to carefully 
>>>>> make - can someone test them for s390, aarch64 and ppc?
>>>>>
>>>>> Tested with tier1 testing, JPRT (all oracle platforms), nsk.jvmti, 
>>>>> monitoring, parallel class loading and g1class unloading tests.
>>>>>
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8186088.01/webrev
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8186088
>>>>
>>>> Is it possible to put the declaration of 
>>>> MacroAssembler::resolve_oop_handle into the shared 
>>>> macroAssembler.hpp file to avoid the need to add it to the platform 
>>>> specific hpp files?
>>>>
>>>> I'm unsure about the OopHandle containing set_atomic. First if it 
>>>> is present then the _obj field should be volatile. But then it also 
>>>> raises the question: if we can race to set this, do we need 
>>>> load-acquire versions of the accessors? Based on the single 
>>>> existing usage I don't think we presently do. But I think it may be 
>>>> cleaner/simpler to not have set_atomic, make the OopHandle 
>>>> immutable, and do the cmpxchg back in the caller code by atomically 
>>>> updating _pd (which should also be volatile even today) with a new 
>>>> OopHandle.
>>>
>>> I couldn't convince the compiler to compile an Atomic::cmpxchg_ptr() 
>>> to an address of OopHandle since it is not a pointer but a struct. The 
>>
>> _pd would have to be a pointer.
>
> Yeah but it's not a pointer.
>>
>>> casting would be wrong.   So I added a resolve_acquire() for the 
>>> protection_domain case in ModuleEntry, and some commentary.
>>
>> For completeness you may also need ptr_acquire().
>
> I can add that too for completeness.

ptr() isn't accessed by multiple threads, so doesn't need acquire. It's 
for the special case that we want to null out an OopHandle in the 
CLD::_handles block.  Only one thread will have access to that OopHandle 
because it just created it.

Coleen

>>
>> The _obj field should be volatile.
>
> This seems like complete overkill because the one case it matters will 
> access through these order access operations.
>
> Coleen
>
>>
>> Thanks,
>> David
>> -----
>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8186088.02/webrev
>>>
>>> thanks,
>>> Coleen
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>>
>>>
>