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

Wed Mar 21 19:33:47 UTC 2018

Thanks, Roman.
Coleen

On 3/21/18 3:23 PM, Roman Kennke wrote:
> Ok, this looks good to me. Thanks!
>
> Roman
>
>
>> 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
>>>>>>>>>>>>
>