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

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


  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.

  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.

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.

 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?

 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.

 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