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

[David Holmes]

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. :)

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

I did check the code that calls this, and the locking seems safe in that 
context.

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