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

[Stefan Karlsson]

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