RFR (S) 8197844: JVMTI GetLoadedClasses should use the Access API

[coleen.phillimore at oracle.com]

Hi Roman, thank you for your comments.

On 3/15/18 9:46 AM, Roman Kennke wrote:
> Am 14.03.2018 um 18:37 schrieb coleen.phillimore at oracle.com:
>> Summary: Make sure the holder of a class loader is accessed during
>> iteration of CLDG
>>
>> This is where we should have put the GC barrier.  This can be cleaned
>> somewhat when we have a WeakHandle holder in the ClassLoaderData, then
>> the code in ensure_loader_alive() becomes _holder.resolve();
>>
>> Tested with tier1-5.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8197844.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8197844
>>
>> Thanks,
>> Coleen
> I am not sure at all that this wouldn't have adverse effects. Suppose a
> GC is iterating all CLDs and marks all of them alive, we'd probably
> never unload it? I probably wouldn't do the keep-alive stuff wholesale
> in the iterator methods.
>
> I've made (and later withdraw) a (IMO) more straightforward patch to
> address the same:
>
> http://cr.openjdk.java.net/~rkennke/8199612/webrev.00/
>
> It has the advantage that it doesn't to a bogus load, just to keep
> something alive. It loads the mirror and at the same time communicates
> the GC to keep it alive. Maybe better?

So when I made this change, I was being conservative.  Anytime we 
iterate over the CLDG, and take out oops to put somewhere else, we need 
to mark that the CLD needs to stay alive for that GC, until the 
somewhere else is walked.  We also have to worry about these classes 
being unloaded if we are still holding metadata after walking the CLDG.

I've gone through all the CLDG::classes_do, loaded_classes_do, 
dictionary_all_entries_do, methods_do, modules_do and packages_do at 
least 100 times.  (also oops_do variants and cld_do).   All these calls 
do something with metadata, and it appears that these places are 
unaffected by GC or class unloading, except the instance in 
jvmtiGetLoadedClasses that Poonam found.

Also if the closure or function passed to the CLDG iterator functions 
stop for a safepoint and unload, the walk will be broken. I didn't find 
any instances of this in the code on my 100 traversals.  My change is 
trying to make it so I don't have to do this analysis anymore.

Poonams, Erik's change and your change puts the responsibility on the 
caller of ClassLoaderDataGraph to keep the class alive that it is 
processing through the walk.   I think I can accept that as a fix, and 
withdraw mine, since we seem to have no instances of bugs in other 
walks.  If it turns out to be something that becomes buggy, I will 
suggest my fix again as an alternative.

This fix doesn't affect the walks done during GC.  As of this morning, 
there are no KlassClosures in GC anymore.   Your concern is well placed 
though because it could help us introduce bugs which could negatively 
affect class unloading.

So on the balance, there's risk in both versions of the fix, so I'll 
withdraw mine.  I think the best fix is yours:

http://cr.openjdk.java.net/~rkennke/8199612/webrev.00/src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp.udiff.html

with Erik's function java_mirror_phantom() that does the RootAccess load.

http://cr.openjdk.java.net/~eosterlund/8197844/webrev.00/src/hotspot/share/oops/klass.cpp.udiff.html

but not have it mark the class_loader, only do below.

+oop Klass::java_mirror_phantom() {
+ // Loading the klass_holder as a phantom oop ref keeps the class alive.
+ // After the holder has been kept alive, it is safe to return the mirror.
+ oop mirror = 
RootAccess<ON_PHANTOM_OOP_REF>::oop_load(k->java_mirror_handle().ptr_raw());

+ return mirror;
+}


I didn't like that Klass was a friend of ClassLoaderData and could get 
to the class_loader field (in case I move it).

Your change here is covered by Kim's work to accessorize jni.

http://cr.openjdk.java.net/~rkennke/8199612/webrev.00/src/hotspot/share/runtime/jniHandles.cpp.udiff.html

So I withdraw and I'll unassign myself and you and/or Erik can take this 
bug back again.

Thanks!
Coleen


> Roman
>