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

[coleen.phillimore at oracle.com]

On 8/18/17 3:31 AM, Christian Thalinger wrote:
>
>> On Aug 15, 2017, at 4:56 AM, coleen.phillimore at oracle.com 
>> <mailto: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 athttp://cr.openjdk.java.net/~coleenp/8186088.03/webrev 
>>>> <http://cr.openjdk.java.net/%7Ecoleenp/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;

No I don't.  I may have had a print function at one point.

Thanks for having a look.
Coleen

>
> ?
>
>>>
>>> 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 
>>>>> <mailto: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 
>>>>>>> <mailto: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 
>>>>>>>> <http://cr.openjdk.java.net/%7Ecoleenp/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 
>>>>>> <http://cr.openjdk.java.net/%7Ecoleenp/8186088.02/webrev>
>>>>>>
>>>>>> thanks,
>>>>>> Coleen
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Coleen
>