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

Kim Barrett kim.barrett at oracle.com
Wed Mar 28 21:12:26 UTC 2018

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

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.

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.

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?

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.

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.

More information about the hotspot-dev mailing list