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

[coleen.phillimore at oracle.com]

On 4/9/18 3:33 PM, Kim Barrett wrote:
>> On Apr 9, 2018, at 9:10 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> I had an extensive conversation offline with Stefan, Robbin and Kim and have reworked this change to walk the CLDG weak roots with the SystemDictionary.  Also, I moved the WeakHandle holder creation into the ClassLoaderData constructor, and made WeakHandle a template class so that multiple weak roots are supported.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8198313.04/webrev
>>
>> This passes tier1-5 tests on linux-x64 and windows-x64.  I've also done another performance test aka. dev-submit with no significant difference.
>>
>> The additional change to make _class_loader oop into an OopHandle or otherwise remove from ClassLoaderData will be done as a separate change because it breaks JFR.
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/weakHandle.inline.hpp
>
> Do resolve and peek actually need a check for NULL?  Could they assert
> non-null instead?  I'm guessing that the places that need these
> operations should never get there with a not-initialized handle.
>
> Oh, but maybe this is needed for the null class loader?  If so, ick!

Yes, it's needed for the ClassLoaderData for the null class loader, but 
it was mostly left over from when the holder was initialized later.

I've added a case for the null class loader in 
ClassLoaderData::holder_phantom() instead and taken out the null 
conditions in resolve and peek.

>
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/weakHandle.hpp
>    60   void clear() { _obj = NULL; }  // for class loading race loss
>
> Is this really needed?  Why not just call release?
>
> Especially since the one caller (CLD::update_holder) looks
> suspiciously like a leak of an OopStorage entry.

This code is wrong.  It should be *_obj = NULL.  I've also moved this to 
be handled in WeakHandle::release().  Even though this loses the assert 
that the released pointer is NULL in OopStorage.  It looks cleaner this 
way.  I also added a comment.

>
> ------------------------------------------------------------------------------
> src/hotspot/share/classfile/classLoaderData.cpp
>   104 ClassLoaderData::ClassLoaderData(Handle h_class_loader, bool is_anonymous) :
>
> Re: Removal of _packages(NULL) from the initializer list, I would have
> instead initialized all the members in the initializer list, and then
> update them when appropriate.  That would eliminate the else-clause
> starting at line 137, and (to me) make the state easier to follow.

If these fields are in the initializer list and then also initialized in 
the constructor, aren't they initialized twice?  I guess even a stupid 
optimizer could figure this out though.  I agree, I'll change this so 
the anonymous case need not NULL out these fields.

>
> ------------------------------------------------------------------------------
> src/hotspot/share/classfile/classLoaderData.cpp
>   978       // need to clear the holder for this case.
>   979       cld->update_holder(Handle());
>
> If WeakHandle::release had
>    RootAccess<ON_PHANTOM_OOP_REF>::oop_store(_obj, (oop)NULL);
> before calling OopStorage::release, then I think the above two lines
> aren't needed?

Yes.  Thanks for the Access call.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/classfile/classLoaderData.cpp
> 1250   int  loaders_processed = 0;
> 1251   int  loaders_removed = 0;
>
> s/int/uint/
>
> And update the log message to use %u rather than %d.

Thanks, fixed.  Hope it doesn't get that large.
>
> ------------------------------------------------------------------------------
> 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
>
> Lost earlier (some previous webrev in this series, I think) fix to the
> comment.

Should have been fixed in webrev.04 but I don't think I fixed the webrev 
after I noticed this.  It'll be in the full webrev.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/classfile/systemDictionary.cpp
> 1018     // Anonymous classes must update ClassLoaderData holder (was host_klass loader)
> 1019     // so that they can be unloaded when the mirror is no longer referenced.
> 1020     k->class_loader_data()->update_holder(Handle(THREAD, k->java_mirror()));
>
> The comment here suggests the holder has been set to something else
> and now we need to update it.  But the CLD constructor only calls
> update_holder for non-anonymous classes.  This suggests that if the
> call to update_holder mentioned above (to clear it) were eliminated,
> then update_holder could be once-only (with an assert) and should be
> called initialize_holder.

Yup. I renamed the function initialize_holder.

This is the incremental change:

http://cr.openjdk.java.net/~coleenp/8198313.incr.05/webrev/index.html

Full changes:

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

Thanks,
Coleen
>
> ------------------------------------------------------------------------------
>