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

[Christian Thalinger]

> On Aug 15, 2017, at 4:56 AM, coleen.phillimore at oracle.com wrote:
> 
> 
> 
> On 8/14/17 8:26 PM, David Holmes wrote:
>> On 15/08/2017 9:46 AM, coleen.phillimore at oracle.com <mailto: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 <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.

Do you still need this:

  32 class outputStream;

?

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