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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Wed Mar 21 19:21:53 UTC 2018 

I've renamed ensure_loader_alive to holder_phantom() and reran jvmti tests.

http://cr.openjdk.java.net/~coleenp/8197844.04/webrev/index.html

Thanks,
Coleen

On 3/21/18 9:12 AM, Erik Österlund wrote:
> Hi Coleen,
>
> On 2018-03-21 13:20, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 3/21/18 5:33 AM, Erik Österlund wrote:
>>> Hi Coleen,
>>>
>>> I like this conservative style. A few comments though:
>>>
>>> 1) I think ensure_loader_alive() should be renamed to 
>>> holder_phantom() or something like that, as it does more than just 
>>> keep the loader alive. It also returns the holder oop. And we have 
>>> had this other naming convention in all other places where we do that.
>>
>> Yes, I could rename this holder_phantom.  I think this would go 
>> nicely with this change that I'm working on for bug
>>
>> https://bugs.openjdk.java.net/browse/JDK-8198313
>> http://cr.openjdk.java.net/~coleenp/8198313.01/webrev/index.html
>
> Great.
>
>>
>>>
>>> 2) I wonder if it would be any trouble to provide an opt-out for the 
>>> keep alive stuff, so that people that explicitly do not want to keep 
>>> things alive, can opt out.
>>
>> It would be trouble right now. :)
>
> Okay. I suppose whoever runs into this (and I know exactly who) will 
> have to implement that themselves instead.
> Looks good.
>
> Thanks,
> /Erik
>
>> thanks,
>> Coleen
>>
>>>
>>> Thanks,
>>> /Erik
>>>
>>> On 2018-03-20 22:49, coleen.phillimore at oracle.com wrote:
>>>>
>>>> Kim and I had some offline discussions about this and he 
>>>> recommended that the ClassLoaderData::ensure_loader_alive function 
>>>> return the oop to handleize while the closure runs on that 
>>>> ClassLoaderData. This is that change.
>>>>
>>>> Tested with jvmti, runtime and jfr tests that use 
>>>> ClassLoaderData::*classes/modules/package iterations.
>>>>
>>>> http://cr.openjdk.java.net/~coleenp/8197844.03/webrev/index.html
>>>>
>>>> thanks,
>>>> Coleen
>>>>
>>>> On 3/19/18 7:38 PM, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>>
>>>>> On 3/19/18 7:32 PM, Roman Kennke wrote:
>>>>>> The change looks good to me.
>>>>>>
>>>>>> I have also verified that Shenandoah does not call into any of the
>>>>>> iterators that might resurrect CLDs by mistake.
>>>>>
>>>>> That's good!  Thank you for verifying this.  These iterators 
>>>>> should now be only runtime iterators.
>>>>> thanks,
>>>>> Coleen
>>>>>
>>>>>>
>>>>>> Thanks, Roman
>>>>>>
>>>>>>
>>>>>>> After reading my own reply and Erik's, I want to reconsider my 
>>>>>>> change
>>>>>>> for this bug.
>>>>>>>
>>>>>>> There are many places that iterate over ClassLoaderDataGraph, 
>>>>>>> and I've
>>>>>>> found another that is buggy in the same way that JVMTI 
>>>>>>> GetLoadedClasses
>>>>>>> should have a G1 barrier.  I've also spent more time trying to 
>>>>>>> find if
>>>>>>> other uses are buggy, with dissatisfying inconclusion.
>>>>>>>
>>>>>>> The safest thing to do is mark loaders that we are iterating 
>>>>>>> over as
>>>>>>> alive.  This alive barrier doesn't affect oops_do, cld_do or
>>>>>>> do_unloading, which are the cases that would be walked by the 
>>>>>>> GC.   It
>>>>>>> also can't include verify, which I've removed from the original 
>>>>>>> change.
>>>>>>>
>>>>>>> I can't assert that the thread isn't a GC thread because
>>>>>>> VM_GC_HeapInspection builds a KlassInfoTable as a GC thread.
>>>>>>>
>>>>>>> I think a klass walk of the ClassLoaderDataGraph when done for 
>>>>>>> profiling
>>>>>>> and assorted runtime code, must necessarily keep the classes alive
>>>>>>> during the interval between GCs.   The class can be unloaded in a
>>>>>>> further GC.
>>>>>>>
>>>>>>> open webrev at 
>>>>>>> http://cr.openjdk.java.net/~coleenp/8197844.02/webrev
>>>>>>>
>>>>>>> This is the same patch with the ensure_loader_alive removed from
>>>>>>> ClassLoaderDataGraph::verify.
>>>>>>>
>>>>>>> thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>> On 3/16/18 11:29 AM, Erik Österlund wrote:
>>>>>>>> Hi Coleen,
>>>>>>>>
>>>>>>>> I for one would like to see your version of the fix, plus some
>>>>>>>> suitable assert to catch bad use of these APIs.
>>>>>>>> The motivation is that it seems unfortunate to expose APIs for 
>>>>>>>> getting
>>>>>>>> Metadata that when retrieved is unstable and unsafe to use 
>>>>>>>> unless you
>>>>>>>> have deep knowledge about GC races and class unloading, and 
>>>>>>>> hope all
>>>>>>>> callsites know that they can't just use the things they retrieved
>>>>>>>> because they are dangerously explosive. The only problem with 
>>>>>>>> the more
>>>>>>>> conservative API is if the GC itself calls into the code and
>>>>>>>> accidentally keeps everything alive preventing class unloading. 
>>>>>>>> But I
>>>>>>>> think a suitable assert could catch that.
>>>>>>>>
>>>>>>>> I'm not 100% sure what the assert condition should be. But I'm
>>>>>>>> thinking something along the lines of:
>>>>>>>> Thread::current()->is_Java_thread() ||
>>>>>>>> Thread::current()->is_VM_thread() &&
>>>>>>>> (!SafepointSynchronize::is_at_safepoint() || 
>>>>>>>> !HeapLock->is_locked())
>>>>>>>> Other suggestions for a good condition are welcome - it is just an
>>>>>>>> example that may or may not work.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> /Erik
>>>>>>>>
>>>>>>>> On 2018-03-16 16:01, coleen.phillimore at oracle.com wrote:
>>>>>>>>> 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
>>>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

More information about the hotspot-runtime-dev mailing list