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

[coleen.phillimore at oracle.com]

Hi Stefan,  Thank you for looking at the code.  I didn't realize that 
you would review this so I've already pushed it, but I can open bugs for 
any issues we decide need to be changed.

On 3/7/18 3:41 AM, Stefan Karlsson wrote:
> Hi Coleen,
>
> It's great that we are converging on one way to scan oops in the CLDs. 
> I have some comments inlined:

Good, yes, we talked about this!
>
> 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.

Right, we've also added tests to do this delegation and with anonymous 
classes (see code around system dictionary change), there are some 
dependencies added for that LambdaForm code generated.  So the number 
isn't quite zero.   The answer below is yes, we have exercised this code.

Since eclipse isn't in daCapo, I don't really have any way to run it.   
I've failed to install it several times.   Let's try to get it added to 
the normal benchmark suite that we run.
>
>>
>> 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?
>

Yes, see above.

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

Hm, yes, you're right.  We need two different contains() functions.   
I'll file a bug to add this back.
thanks,
Coleen
>
> Thanks,
> StefanK
>
>> bug link https://bugs.openjdk.java.net/browse/JDK-8198926
>>
>> Thanks,
>> Coleen
>
>