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

[coleen.phillimore at oracle.com]

Here is an updated webrev with fixes for your comments.

open webrev at http://cr.openjdk.java.net/~coleenp/8186777.03/webrev

Thanks for reviewing and all your help with this!

Coleen

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.
>
> ----------
>
>  #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).
>
> ==========
> 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.
>
> ==========
> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/gc/g1/g1HeapVerifier.cpp.frames.html 
>
>
> Spurious addition:
> +  G1CollectedHeap* _g1h;
>
> ==========
> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/gc/g1/g1OopClosures.hpp.udiff.html 
>
>
> Spurious addition?:
> +  G1CollectedHeap* g1() { return _g1; }
>
> ==========
> 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.
>
> ==========
> 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?
>
> ==========
> 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.
>
> ----------
> +  void do_cld(ClassLoaderData* k);
>
> Rename k?
>
> 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