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

[coleen.phillimore at oracle.com]

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.

thanks,
Coleen
>
> Thanks,
> StefanK
>