RFR: 8247879: Rework WeakHandle and OopHandle to dynamically support different OopStorages

[Erik Österlund]

Hi Stefan,

I really like the direction of this work. Tiny nit:
In oopHandle.hpp maybe include oopsHierarchy.hpp instead of forward 
declaring class oop; Because in product builds it won't be a class, it 
will be an oopDesc*, so not sure how happy compilers are with that 
forward declaration not matching the type.

Otherwise, looks good to me.

Thanks,
/Erik

On 2020-06-22 14:12, Stefan Karlsson wrote:
>
> On 2020-06-19 17:56, coleen.phillimore at oracle.com wrote:
>>
>> https://cr.openjdk.java.net/~stefank/8247879/webrev.01/src/hotspot/share/classfile/classLoaderData.hpp.udiff.html 
>>
>>
>> + WeakHandle _holder; // The oop that determines lifetime of this 
>> class loader
>> + OopHandle _class_loader; // The instance of java/lang/ClassLoader 
>> associated with
>> + // this ClassLoaderData
>>
>>
>> Can you line the member names up?
>>
>>
>> On 6/18/20 6:09 PM, Stefan Karlsson wrote:
>>> Hi all,
>>>
>>> Please review this patch to rework WeakHandle and OopHandle to 
>>> dynamically support different OopStorages.
>>>
>>> https://cr.openjdk.java.net/~stefank/8247879/webrev.01/
>>> https://bugs.openjdk.java.net/browse/JDK-8247879
>>>
>>> Today WeakHandle has a template parameter and specialization of 
>>> get_storage() to get hold of the backing OopStorage. Coleen, ErikÖ 
>>> and I talked about different ways to rewrite that, so that we 
>>> wouldn't have to write specializations for all weak oop storages, 
>>> and so that we don't have to carry around the type information about 
>>> the weak oop storage.
>>>
>>> Instead of writing:
>>> WeakHandle<vm_weak_data> w = WeakHandle<vm_weak_data>::create(obj)
>>>
>>> we could instead write:
>>> WeakHandle w(OopStorageSet::vm_weak(), obj)
>>>
>>> OopHandle is today hard-coded to use OopStorageSet::vm_global(). The 
>>> proposal is to change OopHandle to look like WeakHandle, and allow 
>>> more than one OopStorage. Something that might will become important 
>>> going forward, when we are going to move more things into separate 
>>> OopStorages.
>>>
>>> So, instead of writing:
>>> OopHandle h = OopHandle::create(obj)
>>>
>>> one will have to specify the OopStorage explicitly:
>>> OopHandle h(OopStorageSet::vm_global(), obj)
>>>
>>> This change has a slight drawback that the OopStorage used for the 
>>> allocation of the handle, needs to be provided when the handle is 
>>> released.
>>>
>>> To make this safer, so that you don't use strong OopStorages when 
>>> allocating WeakHandles, or weak OopStorages when allocating 
>>> OopHandles, I've created two subclasses to OopStorage: 
>>> StrongOopStorage and WeakOopStorage. They are just two tiny wrappers 
>>> around OopStorage, to allow us ensure correct usage during compile 
>>> time. The oopStorageSet.hpp files seems to be intentionally written 
>>> to *not* know about the OopStorage implementation. To do proper 
>>> casting of the OopStorage*s I need to include the definition of 
>>> StrongOopStorage and WeakOopStorage. I've therefore moved all 
>>> OopStorageSet::<name_of_oop_storage>() getters out to an inline 
>>> header. This adds a lot of new includes. If we don't think that it's 
>>> important to inline these, then I can move it over to the cpp file 
>>> instead.
>>
>> I don't like these includes.  In the places where we allocate and 
>> release OopHandle/WeakHandles, where we need to refer to these 
>> storages, performance isn't important to avoid a call.  I don't know 
>> about all of the GC places.
>>
>> I also don't see a large benefit to type safety that these new 
>> classes provide, to be honest, since there aren't many call sites 
>> where we create and release these handles.  We're more likely to mix 
>> different strong oop storages (allocate from one and release from 
>> another), once we have more than one strong OopStorage. 
>> OopStorage::release has a check that the memory is in that storage (I 
>> hope), so there already should be a runtime check.
>>
>> I'll defer to whatever Kim thinks though.  This doesn't seem worth 
>> having to me though.
>>
>> Otherwise, the changes look good.
>
> I reverted all the Strong/Weak designations. Is this OK?:
> https://cr.openjdk.java.net/~stefank/8247879/webrev.02
>
> StefanK
>
>>
>> thanks,
>> Coleen
>>>
>>> Thanks,
>>> StefanK
>>>
>>