RFR (L) 8186777: Make Klass::_java_mirror an OopHandle

Fri Sep 29 13:00:31 UTC 2017

On 9/29/17 6:41 AM, Stefan Karlsson wrote:
> Hi Coleen,
>
> I started looking at this, but will need a second round before I've 
> fully reviewed the GC parts.
>
> Here are some nits that would be nice to get cleaned up.
>
> ==========
> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/classfile/classLoaderData.cpp.frames.html 
>
>
>  788     record_modified_oops();  // necessary?
>
> This could be removed. Only G1 cares about deleted "weak" references.
>
> Or we can wait until ErikÖ's GC Barrier Interface is in place and 
> remove it then.

I'll remove it now.  I didn't know whether deleting a handle required 
another CLD walk in young collections, but I think you're saying it does 
not.

>
> ----------
>
>  #ifdef CLD_DUMP_KLASSES
>    if (Verbose) {
>      Klass* k = _klasses;
>      while (k != NULL) {
> -      out->print_cr("klass " PTR_FORMAT ", %s, CT: %d, MUT: %d", k, 
> k->name()->as_C_string(),
> -          k->has_modified_oops(), k->has_accumulated_modified_oops());
> +      out->print_cr("klass " PTR_FORMAT ", %s", k, 
> k->name()->as_C_string());
>        assert(k != k->next_link(), "no loops!");
>        k = k->next_link();
>      }
>    }
>  #endif  // CLD_DUMP_KLASSES
>
> Pre-existing: I don't think this will compile if you turn on 
> CLD_DUMP_KLASSES. k must be p2i(k).

Fixed.
>
> ==========
> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/classfile/classLoaderData.hpp.udiff.html 
>
>
> +  // Remembered sets support for the oops in the class loader data.
> +  jbyte _modified_oops;             // Card Table Equivalent (YC/CMS 
> support)
> +  jbyte _accumulated_modified_oops; // Mod Union Equivalent (CMS 
> support)
>
> We should create a follow-up bug to change these jbytes to bools.

I agree.  I could do that here, in this change and retest.   I noticed 
that I didn't initialize _accumulate_modified_oops in the CLD constructor.

>
> ==========
> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/gc/g1/g1HeapVerifier.cpp.frames.html 
>
>
> Spurious addition:
> +  G1CollectedHeap* _g1h;

Yes, I don't need that field in VerifyCLDClosure.  Removed.
>
> ==========
> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/gc/g1/g1OopClosures.hpp.udiff.html 
>
>
> Spurious addition?:
> +  G1CollectedHeap* g1() { return _g1; }

Removed, that was leftover from a previous version.  Thanks for noticing it.
>
> ==========
> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/gc/parallel/psScavenge.inline.hpp.patch 
>
>
>    PSPromotionManager* _pm;
> -  // Used to redirty a scanned klass if it has oops
> +  // Used to redirty a scanned cld if it has oops
>    // pointing to the young generation after being scanned.
> -  Klass*             _scanned_klass;
> +  ClassLoaderData*             _scanned_cld;
>
> Indentation.

Fixed.
>
> ==========
> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/gc/parallel/psTasks.cpp.frames.html 
>
>
>   80     case class_loader_data:
>   81     {
>   82       PSScavengeCLDClosure ps(pm);
>   83       ClassLoaderDataGraph::cld_do(&ps);
>   84     }
>
> Would you mind changing the name ps to cld_closure?

Not at all.   Fixed.

>
> ==========
> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/gc/shared/genOopClosures.hpp.patch 
>
>
> +  OopsInClassLoaderDataOrGenClosure*   _scavenge_closure;
>    // true if the the modified oops state should be saved.
>    bool                     _accumulate_modified_oops;
>
> Indentation.

I moved it to align with _scavenge closure, although I think the coding 
standard algorithm is to align if there are no intervening comments, and 
not otherwise.  But it looks a bit better aligned.

>
> ----------
> +  void do_cld(ClassLoaderData* k);
>
> Rename k?

Fixed.

Thanks for reading through this.  I tried to make it completely nit free 
but I missed a k. argh.  :)

Coleen

>
> Thanks,
> StefanK
>
> On 2017-09-28 23:36, coleen.phillimore at oracle.com wrote:
>>
>> Thank you to Stefan Karlsson offlist for pointing out that the 
>> previous .01 version of this webrev breaks CMS in that it doesn't 
>> remember ClassLoaderData::_handles that are changed and added while 
>> concurrent marking is in progress.  I've fixed this bug to move the 
>> Klass::_modified_oops and _accumulated_modified_oops to the 
>> ClassLoaderData and use these fields in the CMS remarking phase to 
>> catch any new handles that are added.   This also fixes this bug 
>> https://bugs.openjdk.java.net/browse/JDK-8173988 .
>>
>> In addition, the previous version of this change removed an 
>> optimization during young collection, which showed some uncertain 
>> performance regression in young pause times, so I added this 
>> optimization back to not walk ClassLoaderData during young 
>> collections if all the oops are old.  The performance results of 
>> SPECjbb2015 now are slightly better, but not significantly.
>>
>> This latest patch has been tested on tier1-5 on linux x64 and windows 
>> x64 in mach5 test harness.
>>
>> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/
>>
>> Can I get at least 3 reviewers?  One from each of the compiler, gc, 
>> and runtime group at least since there are changes to all 3.
>>
>> Thanks!
>> Coleen
>>
>>
>> On 9/6/17 12:04 PM, coleen.phillimore at oracle.com wrote:
>>> Summary: Add indirection for fetching mirror so that GC doesn't have 
>>> to follow CLD::_klasses
>>>
>>> Thank you to Tom Rodriguez for Graal changes and Rickard for the C2 
>>> changes.
>>>
>>> Ran nightly tests through Mach5 and RBT.   Early performance testing 
>>> showed good performance improvment in GC class loader data 
>>> processing time, but nmethod processing time continues to dominate. 
>>> Also performace testing showed no throughput regression.   I'm 
>>> rerunning both of these performance testing and will post the numbers.
>>>
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8186777
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8186777.01/webrev
>>>
>>> Thanks,
>>> Coleen