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

[Stefan Karlsson]

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

If you want. They were not aligned, and none of the other members are 
alined, so the request is a bit unexpected.

StefanK

> 
> 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.
> 
> thanks,
> Coleen
>>
>> Thanks,
>> StefanK
>>
>