RFR (M) 8198926: Move ClassLoaderData::_dependencies to ClassLoaderData::_handles

[Stefan Karlsson]

Hi Coleen,

It's great that we are converging on one way to scan oops in the CLDs. I 
have some comments inlined:

On 2018-03-05 02:25, coleen.phillimore at oracle.com wrote:
> Summary: Move dependency creation and cleaned up logging
>
> This change moves dependency creation to the ClassLoaderData::_handles 
> area so that it is scanned with _handles for GC.   The presence of the 
> dependency (either class loader or mirror for anonymous class), will 
> keep GC from unloading that dependency before unloading the 
> ClassLoaderData that has it. This replaces the objArrayOop field where 
> the array was populated with {dependency, next}.  Because the 
> objArrayOop could throw OOM while creation we needed to be careful 
> where it was called because it could call GC at inconvenient times, 
> and pass TRAPS.   A lot of this change removes the TRAPS parameter 
> from dependency and recording class loader data calls.
>
> The part of this change removes the redundant logging of 
> ClassLoaderData to call print_value_on, renames dump to print to 
> follow convention, and removes a call to Java to get toString() for 
> instances of class loader for logging.  The latter is unnecessary 
> since print_value_on() will print the address of the instance 
> (toString appends the hash number to the class name, ie. it's not more 
> informative), and this is a terrible place for an upcall to Java.
>
> Some of the logging is to verify that dependencies added are fairly 
> rare, since adding them are unnecessary for class loader parents (that 
> is through class loader delegation) or class loaders that are not 
> unloaded at all.   I verified this is true with daCapo and 
> SPECjbb2015, and runThese jck tests.

During the permgen removal we saw this as well. However, there are 
applications that delegate, but not through the parents. One example, is 
eclipse. If I understand the change correctly, when we have this 
situation, we now scan the entire _handles area to see if the dependency 
has already been registered.

>
> Also tested performance with internal refworkload benchmarks, since 
> the original reason for the objArray was to use regular gc marking 
> when adding a dependency.  There was no decrease in performance, again 
> since these are rare.

Did you run anything that actually exercised the 'if 
(!_handles.contains(to)) { ...}' path? Did you measure the performance 
for those benchmarks?

>   The nice thing is if the number of dependencies is zero there are 
> less of these objarray[2] objects, which were allocated eagerly one 
> for each class loader.

That's nice!

>
> This was also tested with mach5 tier1-5 with no failures.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8198926.01/webrev

http://cr.openjdk.java.net/~coleenp/8198926.01/webrev/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html

  void ClassLoaderData::remove_handle(OopHandle h) {
    assert(!is_unloading(), "Do not remove a handle for a CLD that is unloading");
    oop* ptr = h.ptr_raw();
    if (ptr != NULL) {
- assert(_handles.contains(ptr), "Got unexpected handle " PTR_FORMAT, 
p2i(ptr));
+ assert(_handles.contains(*ptr), "Got unexpected handle " PTR_FORMAT, 
p2i(ptr));


With this change have now started too look for the oop and not the oop*. 
That seems incorrect to me. I think the purpose of this test to check 
that the OopHandle actually belongs to this _handles container, and not 
some other CLD's _handles.

Thanks,
StefanK

> bug link https://bugs.openjdk.java.net/browse/JDK-8198926
>
> Thanks,
> Coleen