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

[Stefan Karlsson]

On 2020-06-22 17:05, Erik Österlund wrote:
> Hi Stefan,
> 
> I really like the direction of this work.

Thanks. We've heard some offline concerns voiced about need to provide 
an OopStorage to the release(...) function. Given that, I'm going to 
wait with this patch until that has been discussed/resolved.

  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.

Thanks. I fixed this in another patch, but it should of course be in 
this patch. I also found this pre-existing oddity that we might want to 
cleanup in this patch:

  class WeakHandle {
   public:
   private:


> 
> Otherwise, looks good to me.

Thanks,
StefanK

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