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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Mon Apr 9 22:21:47 UTC 2018

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 classloader 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::updateholder) 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 hclassloader, bool isanonymous) : 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->updateholder(Handle()); If WeakHandle::release had RootAccess::oopstore(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 loadersprocessed = 0; 1251 int loadersremoved = 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 classloader; // 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 hostklass loader) 1019 // so that they can be unloaded when the mirror is no longer referenced. 1020 k->classloaderdata()->updateholder(Handle(THREAD, k->javamirror())); 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 updateholder for non-anonymous classes. This suggests that if the call to updateholder mentioned above (to clear it) were eliminated, then updateholder could be once-only (with an assert) and should be called initializeholder.

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

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

More information about the hotspot-dev mailing list