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

[coleen.phillimore at oracle.com]

On 6/22/20 4:31 PM, Stefan Karlsson wrote:
>
>
> 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.
>
I've changed my JVMTI change to use OopHandles based on your patch and 
the forward declaration of oop in oopHandle.hpp, does cause a build 
error for one of the targets, which might use precompiled headers:

/workspace/open/src/hotspot/share/oops/oopHandle.hpp:28:7: error: using typedef-name 'oop' after 'class'
[2020-06-22T18:02:13,683Z]    28 | class oop;
[2020-06-22T18:02:13,683Z]       |       ^~~
/workspace/open/src/hotspot/share/oops/oopsHierarchy.hpp:45:43: note: 'oop' has a previous declaration here
[2020-06-22T18:02:13,686Z]    45 | typedef class oopDesc*                    oop;
[2020-06-22T18:02:13,687Z]       |                                           ^~~

Coleen


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