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

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
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

Thanks,
Coleen


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

serguei.spitsyn@oracle.com
Hi Coleen,

The fix looks good.

Thanks,
Serguei


On 8/11/17 14:34, [hidden email] 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
>
> Thanks,
> Coleen
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by coleen.phillimore
Hi Coleen,

On 12/08/2017 7:34 AM, [hidden email] 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.

Thanks,
David

> Thanks,
> Coleen
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by serguei.spitsyn@oracle.com
Thank you Serguei!
Coleen

On 8/12/17 3:59 AM, [hidden email] wrote:

> Hi Coleen,
>
> The fix looks good.
>
> Thanks,
> Serguei
>
>
> On 8/11/17 14:34, [hidden email] 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
>>
>> Thanks,
>> Coleen
>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by David Holmes


On 8/13/17 9:04 PM, David Holmes wrote:

> Hi Coleen,
>
> On 12/08/2017 7:34 AM, [hidden email] 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 did look at that but the platform independent macroAssembler.hpp file
is empty and only includes the platform dependent one.

The platform dependent one is a full class declaration, so isn't
additive of the platform independent one unfortunately.  There are a lot
of duplication which would be nice to clean up.

>
> 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 had trouble writing this in the caller, which is where I would have
preferred it also.   But I certainly don't want to make resolve() do a
load_acquire for this case.  I'll try to rewrite it.

Coleen
>
> Thanks,
> David
>
>> Thanks,
>> Coleen
>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by David Holmes


On 8/13/17 9:04 PM, David Holmes wrote:

> Hi Coleen,
>
> On 12/08/2017 7:34 AM, [hidden email] 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
casting would be wrong.   So I added a resolve_acquire() for the
protection_domain case in ModuleEntry, and some commentary.

open webrev at http://cr.openjdk.java.net/~coleenp/8186088.02/webrev

thanks,
Coleen

>
> Thanks,
> David
>
>> Thanks,
>> Coleen
>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jiangli Zhou
Hi Coleen,

This looks good to me. I noticed a small issue, which isn’t caused by your change, but related to ClassLoaderData::remove_handle_unsafe(). There could be a small memory leak in the ChunkedHandleList when remove_handle_unsafe() is used to remove a handle under race condition. ClassLoaderData::remove_handle_unsafe() only sets the *_data[idx] to NULL in the list. However, that _data[idx] is never reused and set to another oop after the point. Currently the race condition could only happen during setting one of the shared ProtectionDomain, so the leak is small. To avoid the memory leak, ChunkedHandleList::add() could try to reuse a NULL _data[idx] in the list, but that requires to scan the _data[] array of all chunks. Since this is not part of your change, I think we can handle that separately.

Thanks,
Jiangli

> On Aug 14, 2017, at 8:38 AM, [hidden email] wrote:
>
>
>
> On 8/13/17 9:04 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 12/08/2017 7:34 AM, [hidden email] 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 casting would be wrong.   So I added a resolve_acquire() for the protection_domain case in ModuleEntry, and some commentary.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8186088.02/webrev
>
> thanks,
> Coleen
>
>>
>> Thanks,
>> David
>>
>>> Thanks,
>>> Coleen
>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

serguei.spitsyn@oracle.com
In reply to this post by coleen.phillimore
Hi Coleen,

Looks good.

http://cr.openjdk.java.net/~coleenp/8186088.02/webrev/src/share/vm/oops/oopHandle.hpp.html

   There is unneeded space after the const:

   50   oop resolve() const         { return (_obj == NULL) ? (oop)NULL : *_obj; }


Thanks,
Serguei


On 8/14/17 08:38, [hidden email] wrote:

>
>
> On 8/13/17 9:04 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 12/08/2017 7:34 AM, [hidden email] 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
> casting would be wrong.   So I added a resolve_acquire() for the
> protection_domain case in ModuleEntry, and some commentary.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8186088.02/webrev
>
> thanks,
> Coleen
>
>>
>> Thanks,
>> David
>>
>>> Thanks,
>>> Coleen
>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by coleen.phillimore
Hi Coleen,

On 15/08/2017 1:38 AM, [hidden email] wrote:

>
>
> On 8/13/17 9:04 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 12/08/2017 7:34 AM, [hidden email] 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
>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore


On 8/14/17 4:36 PM, David Holmes wrote:

> Hi Coleen,
>
> On 15/08/2017 1:38 AM, [hidden email] wrote:
>>
>>
>> On 8/13/17 9:04 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 12/08/2017 7:34 AM, [hidden email] 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.
>
> 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
>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore


On 8/14/17 6:27 PM, [hidden email] wrote:

>
>
> On 8/14/17 4:36 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 15/08/2017 1:38 AM, [hidden email] wrote:
>>>
>>>
>>> On 8/13/17 9:04 PM, David Holmes wrote:
>>>> Hi Coleen,
>>>>
>>>> On 12/08/2017 7:34 AM, [hidden email] 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
>>>>>
>>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by David Holmes

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 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, [hidden email] wrote:
>>
>>
>> On 8/13/17 9:04 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 12/08/2017 7:34 AM, [hidden email] 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
>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by coleen.phillimore
On 15/08/2017 8:27 AM, [hidden email] wrote:

> On 8/14/17 4:36 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 15/08/2017 1:38 AM, [hidden email] wrote:
>>>
>>>
>>> On 8/13/17 9:04 PM, David Holmes wrote:
>>>> Hi Coleen,
>>>>
>>>> On 12/08/2017 7:34 AM, [hidden email] 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.
>>
>> The _obj field should be volatile.
>
> This seems like complete overkill because the one case it matters will
> access through these order access operations.

?? Any field accessed via race conditions, and which thus will be
accessed via orderAccess and Atomics should always be declared volatile!

Cheers,
David

> Coleen
>
>>
>> Thanks,
>> David
>> -----
>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8186088.02/webrev
>>>
>>> thanks,
>>> Coleen
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by coleen.phillimore
On 15/08/2017 9:46 AM, [hidden email] 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, [hidden email] wrote:
>>>
>>>
>>> On 8/13/17 9:04 PM, David Holmes wrote:
>>>> Hi Coleen,
>>>>
>>>> On 12/08/2017 7:34 AM, [hidden email] 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
>>>>>
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore


On 8/14/17 8:26 PM, David Holmes wrote:
> On 15/08/2017 9:46 AM, [hidden email] 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, [hidden email] wrote:
>>>>
>>>>
>>>> On 8/13/17 9:04 PM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> On 12/08/2017 7:34 AM, [hidden email] 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
>>>>>>
>>>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by Jiangli Zhou


On 8/14/17 1:41 PM, Jiangli Zhou wrote:
> Hi Coleen,
>
> This looks good to me. I noticed a small issue, which isn’t caused by your change, but related to ClassLoaderData::remove_handle_unsafe(). There could be a small memory leak in the ChunkedHandleList when remove_handle_unsafe() is used to remove a handle under race condition. ClassLoaderData::remove_handle_unsafe() only sets the *_data[idx] to NULL in the list. However, that _data[idx] is never reused and set to another oop after the point. Currently the race condition could only happen during setting one of the shared ProtectionDomain, so the leak is small. To avoid the memory leak, ChunkedHandleList::add() could try to reuse a NULL _data[idx] in the list, but that requires to scan the _data[] array of all chunks. Since this is not part of your change, I think we can handle that separately.

Hi Jiangli,

Can you have a look at the latest RFR?  I removed this
remove_handle_unsafe.  I probably need to add it back again to fix a
longstanding bug with redefinition.  When they were JNIHandleBlocks we
did reuse entries sometimes, but now that they're chunked array. I'm not
sure if they can do that without breaking GC (CMS).

Thanks,
Coleen

>
> Thanks,
> Jiangli
>
>> On Aug 14, 2017, at 8:38 AM, [hidden email] wrote:
>>
>>
>>
>> On 8/13/17 9:04 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 12/08/2017 7:34 AM, [hidden email] 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 casting would be wrong.   So I added a resolve_acquire() for the protection_domain case in ModuleEntry, and some commentary.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8186088.02/webrev
>>
>> thanks,
>> Coleen
>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Christian Thalinger-4
In reply to this post by coleen.phillimore

> On Aug 15, 2017, at 4:56 AM, [hidden email] wrote:
>
>
>
> On 8/14/17 8:26 PM, David Holmes wrote:
>> On 15/08/2017 9:46 AM, [hidden email] <mailto:[hidden email]> 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, [hidden email] wrote:
>>>>>
>>>>>
>>>>> On 8/13/17 9:04 PM, David Holmes wrote:
>>>>>> Hi Coleen,
>>>>>>
>>>>>> On 12/08/2017 7:34 AM, [hidden email] 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

Loading...