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

[coleen.phillimore at oracle.com]

On 6/22/20 7:16 AM, 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?
>>
>
> If you want. They were not aligned, and none of the other members are 
> alined, so the request is a bit unexpected.

It's just that these two fields are next to each other and off by one, 
which was visually distracting to me.  Since there are comments 
separating groups of fields, aligning all of them seems unnecessary for 
visibility.  Just these two.

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