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

[coleen.phillimore at oracle.com]

On 8/14/17 8:26 PM, David Holmes wrote:
> On 15/08/2017 9:46 AM, coleen.phillimore at oracle.com wrote:
>>
>> Better yet, I removed the _pd field assignment from being 
>> cmpxchg/conditional and do it under the metaspace_lock which we take 
>> out anyway to add the handle.   How is this?
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8186088.03/webrev
>
> I like the fact OopHandle no longer has atomics. :)

Yes, this solved a bunch of problems.
>
> Sorry I'm being a bit dense today when it comes to this code:
>
> ! void ClassLoaderData::init_handle_locked(OopHandle& dest, Handle h) {
> !   MutexLockerEx ml(metaspace_lock(), Mutex::_no_safepoint_check_flag);
> !   if (dest.resolve() != NULL) {
> !     return;
> !   } else {
> !     dest = _handles.add(h());
> !   }
>   }
>
> I would have expected to see something like dest.set(x). I'm not sure 
> what magic operator= is doing behind the scenes here ??

It's doing a bitwise copy.
>
> I did check the code that calls this, and the locking seems safe in 
> that context.
>

Thanks.  It's a lot better now.
Coleen
> Thanks,
> David
>
>> I reran the tests that use this code.
>>
>> thanks,
>> Coleen
>>
>> 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.
>>>
>>>> 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().
>>>
>>> The _obj field should be volatile.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8186088.02/webrev
>>>>
>>>> thanks,
>>>> Coleen
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>>
>>>>
>>