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