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

[Roman Kennke]

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