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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Fri Mar 30 17:53:02 UTC 2018

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 isalive 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::vmweakoopstorage()->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 classloader 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::cleanimplementorslist(BoolObjectClosure* isalive) { 1904   assert(classloaderdata()->isalive(), "this klass should be live"); ... 1909         if (!impl->isloaderalive(isalive)) { I'm kind of surprised we still need the isalive closure here. But there are no changes in isloaderalive.  I think I'm not understanding something. We do not need the isalive closure here.   I have a follow on change in my patch queue that removes these. ------------------------------------------------------------------------------ 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 [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 classloader;          // 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.isnull()? If not, e.g. if !holder.isnull() 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::isnull() a bit confusing, because I keep thinking it's about the value of *handle.obj rather than handle.obj. isnull() is for a holder that hasn't been set yet or is zero as for thenullclassloaderdata(). This case should definitely be isnull(). ------------------------------------------------------------------------------ src/hotspot/share/classfile/classLoaderData.cpp 632   bool alive = keepalive()         // null class loader and incomplete anonymous klasses. 633       || holder.isnull() 634       || (holder.peek() != NULL);  // not cleaned by weak reference processing I was initially guessing that holder.isnull() was for null class loader and/or anonymous classes, but that's covered by the preceeding keepalive().  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 ------------------------------------------------------------------------------

More information about the hotspot-dev mailing list