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

[Kim Barrett]

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

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

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

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

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

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

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

------------------------------------------------------------------------------