RFR (M) 8198313: Wrap holder object for ClassLoaderData in a WeakHandle

[coleen.phillimore at oracle.com]

Hi Kim,
Thank you for reviewing this.

On 3/28/18 5:12 PM, Kim Barrett wrote:
>> On Mar 26, 2018, at 1:26 PM, coleen.phillimore at oracle.com wrote:
>>
>> Summary: Use WeakHandle for ClassLoaderData::_holder so that is_alive closure is not needed
>>
>> The purpose of WeakHandle is to encapsulate weak oops within the runtime code in the vm.  The class was initially written by StefanK.   The weak handles are pointers to OopStorage.   This code is a basis for future work to move direct pointers to the heap (oops) from runtime structures like the StringTable, into pointers into an area that the GC efficiently manages, in parallel and/or concurrently.
>>
>> Tested with mach5 tier 1-5.  Performance tested with internal dev-submit performance tests, and locally.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8198313.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8198313
>>
>> Thanks,
>> Coleen
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/weakHandle.cpp
>
>    59 void WeakHandle::release() const {
>    60   Universe::vm_weak_oop_storage()->release(_obj);
>
> Is WeakHandle::release ever called with a handle that has not been
> cleared by GC?  The only caller I found is ~ClassLoaderData.  Do we
> ever construct a CLD with filled-in holder, and then decide we don't
> want the CLD after all?  I'm thinking of something like an error
> during class loading or the like, but without much knowledge.

We call WeakHandle::release in ~ClassLoaderData only.  The oop is always 
null.   There's a race when adding the ClassLoaderData to the 
class_loader oop.  If we win this race, we create the WeakHandle. See 
lines 982-5 of this change.

I wanted to avoid creating the WeakHandle and destroying it if we lose 
this race, so I did not create it in the ClassLoaderData constructor.
I have a follow-on change that moves it there however.

>
> ------------------------------------------------------------------------------
> src/hotspot/share/classfile/classLoaderData.cpp
>    59 #include "gc/shared/oopStorage.hpp"
>
> Why is this include needed?  Maybe I missed something, but it looks
> like all the OopStorage usage is wrapped up in WeakHandle.

It is not needed, removed.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/instanceKlass.cpp
> 1903 void InstanceKlass::clean_implementors_list(BoolObjectClosure* is_alive) {
> 1904   assert(class_loader_data()->is_alive(), "this klass should be live");
> ...
> 1909         if (!impl->is_loader_alive(is_alive)) {
>
> I'm kind of surprised we still need the is_alive closure here. But
> there are no changes in is_loader_alive.  I think I'm not
> understanding something.

We do not need the is_alive closure here.   I have a follow on change in 
my patch queue that removes these.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/classfile/classLoaderData.hpp
>   224   oop _class_loader;          // oop used to uniquely identify a class loader
>   225                               // class loader or a canonical class path
>
> [Not part of the change, but adjacent to one, so it caught my eye.]
>
> "class loader \n class loader" in the comment looks redundant?

That was left over from the early days.  Rewrote as below.  I have a 
change to remove this later too.

   oop _class_loader;          // The instance of java/lang/ClassLoader 
associated with
                                         // this ClassLoaderData

>
> ------------------------------------------------------------------------------
> src/hotspot/share/classfile/classLoaderData.cpp
>   516   assert(_holder.peek() == NULL, "never replace holders");
>
> I think peek is the wrong test here.  Shouldn't it be _holder.is_null()?
> If not, e.g. if !_holder.is_null() can be true here, then I think
> that would be a leak when _holder is (re)assigned.
>
> Of course, this goes back to my earlier private comment that I find
> WeakHandle::is_null() a bit confusing, because I keep thinking it's
> about the value of *_handle._obj rather than _handle._obj.

is_null() is for a holder that hasn't been set yet or is zero as for 
the_null_class_loader_data().

This case should definitely be is_null().
>
> ------------------------------------------------------------------------------
> src/hotspot/share/classfile/classLoaderData.cpp
>   632   bool alive = keep_alive()         // null class loader and incomplete anonymous klasses.
>   633       || _holder.is_null()
>   634       || (_holder.peek() != NULL);  // not cleaned by weak reference processing
>
> I was initially guessing that _holder.is_null() was for null class
> loader and/or anonymous classes, but that's covered by the preceeding
> keep_alive().  So I don't know why a null holder => alive.

Yes, I believe it is redundant.   Or did I add it for a special case 
that I can't remember.  I'll verify.

Thanks,
Coleen

>
> ------------------------------------------------------------------------------
>