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

[Erik Österlund]

Hi Coleen,

Sure, go ahead. Looking forward to seeing your solution.

Thanks,
/Erik

On 2018-02-15 17:16, coleen.phillimore at oracle.com wrote:
>
> Hi Erik,
>
> I wanted to fix this differently and I don't like that Klass is a 
> friend of ClassLoaderData especially.
>
> The patch in jvmtiGetLoadedClasses.cpp was a minimal patch which is 
> easy for backporting, but in discussions with Kim and Stefan, I think 
> the problem is larger.  When we iterate through ClassLoaderDataGraph 
> we don't want any of the loaders to get unlinked if there's a 
> safepoint in the function or closure that we call, or else the list 
> will get broken.  I don't think we do that now but we should guard 
> against it.  I think the CLDG iterator should have the barrier on the 
> holder, which is good because then it doesn't have to be friends with 
> itself.  We have a bunch of ad-hoc iteration of the graph which should 
> be unified into an iterator.
>
> I'd like to work on a different patch if that is ok with you.
>
> Thanks,
> Coleen
>
> On 2/15/18 5:04 AM, Erik Österlund wrote:
>> Hi,
>>
>> The JVMTI GetLoadedClasses function retrieves, as the name suggests, 
>> all loaded classes. However, to be able to safely return their 
>> mirrors in handles during potential concurrent marking, G1 requires 
>> an SATB enqueue barrier to shade these mirrors grey that were not 
>> part of the snapshot at the beginning, to prevent the classes from 
>> being unloaded.
>>
>> The current solution is to explicitly SATB enqueue the mirror of the 
>> retrieved classes with the usual #ifdef blah if (UseG1GC) jazz.
>>
>> My proposed solution is to instead use the Access API to make the 
>> code more modular. In the Access API, this operation corresponds to 
>> loading the mirror with ON_PHANTOM_OOP_REF. Also, an observation from 
>> my part is that usually when we keep classes from unloading, we keep 
>> the klass_holder alive, rather than the mirror. For all Klasses 
>> except anonymous instanceKlasses, the holder is the class loader. For 
>> anonymous instanceKlasses, the holder is the mirror. As the rest of 
>> the code tends to prevent class unloading by keeping the holder 
>> alive, I would prefer to keep the holder alive instead of the mirror, 
>> unless there is a good reason to keep the mirror alive instead of the 
>> holder.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8197844
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8197844/webrev.00/
>>
>> This change has made it through hs-tier1-5 and jdk-tier1-3.
>>
>> Thanks,
>> /Erik
>