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

[coleen.phillimore at oracle.com]

I have an incremental and full .02 version with the changes discussed here.

open webrev at http://cr.openjdk.java.net/~coleenp/8198313.02.incr/webrev
open webrev at http://cr.openjdk.java.net/~coleenp/8198313.02/webrev

These have been retested on x86, all hotspot jtreg tests.
thanks,
Coleen

On 3/29/18 12:37 PM, coleen.phillimore at oracle.com wrote:
>
> 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
>
>>
>> ------------------------------------------------------------------------------ 
>>
>>
>