RFR: 8075955: Replace the macro based implementation of oop_oop_iterate with a template based solution

[Stefan Karlsson]

Hi,

Please review this patch to replace the macro based implementation of 
oop_oop_iterate with a template based solution.

http://cr.openjdk.java.net/~stefank/8075955/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8075955

Summary of the changes:

- Replace the macro implementation of the different oop_oop_iterate 
functions with template functions. The implementation is moved out from 
the *Klass.cpp files into the *Klass.inline.hpp files, to be able to 
generate the specialized oop_oop_iterate functions in suitable GC 
specific files.

See the old macro implementation:
  http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/instanceKlass.cpp.udiff.html
  http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/instanceMirrorKlass.cpp.udiff.html
  http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/instanceClassLoaderKlass.cpp.udiff.html
  http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/instanceRefKlass.cpp.udiff.html
  http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/typeArrayKlass.cpp.udiff.html
  http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/objArrayKlass.cpp.udiff.html

That has now been converted into template functions:
  http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/instanceKlass.inline.hpp.html
  http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/instanceMirrorKlass.inline.hpp.html
  http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/instanceClassLoaderKlass.inline.hpp.html
  http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/instanceRefKlass.inline.hpp.html
  http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/typeArrayKlass.inline.hpp.html
  http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/objArrayKlass.inline.hpp.udiff.html

There's still a closure specialization layer implemented with macros to 
support the code to allows the code to pass down the concrete closure 
type, past the virtual Klass::oop_oop_iterate_nv(...) call. The macros 
just calls the new template functions:

  191 #define InstanceKlass_OOP_OOP_ITERATE_DEFN(OopClosureType, 
nv_suffix)              \
  192 int InstanceKlass::oop_oop_iterate##nv_suffix(oop obj, 
OopClosureType* closure) {  \
  193   return oop_oop_iterate<nvs_to_bool(nv_suffix)>(obj, 
closure);                    \
  194 }

  We might want to remove this in a future patch, by providing our own 
dispatch mechanism.


- Split the generation of the specialized oop_oop_iterate definitions, 
so that we keep code from different GCs separated. Before this patch, 
code from all GCs were generated into instanceKlass.cpp, 
instanceMirrorKlass.cpp, instanceClassLoaderKlass.cpp, 
instanceRefKlass.cpp, typeArrayKlass.cpp, and objArrayKlass.cpp.

Now the definitions are generated into:
  G1: 
http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/gc_implementation/g1/g1OopClosures.cpp.udiff.html
  CMS: 
http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/gc_implementation/concurrentMarkSweep/cmsOopClosures.cpp.html
  ParNew: 
http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/gc_implementation/parNew/parOopClosures.cpp.html
  Serial: 
http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/memory/genOopClosures.cpp.html


- The other GCs don't use use the above mentioned closure 
specialization, that is, they don't call obj->oop_iterate(&cl) to follow 
the objects. Instead they have their own "visitor" functions located in 
the oopDesc and *Klass classes. For example, Parallel Scavenge calls 
obj->push_contents(...), which calls Klass::oop_push_contents(...), to 
follow outgoing pointers after an object has been moved. These visitor 
functions used to use the oop iterate macros and pass down snippets of 
code to be applied to each oop*. This has now been changed to use use 
closures and the new oop_oop_iterate template functions.

The implementation of these object visitor functions have been moved out 
from the *Klass.cpp files and into the GCs that the functions support.

Using Parallel Scavenge as and example:

The implementation to handle the references out of a copied object was 
located in:
http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/instanceKlass.cpp.udiff.html

-void InstanceKlass::oop_push_contents(PSPromotionManager* pm, oop obj) {
-  InstanceKlass_OOP_MAP_REVERSE_ITERATE( \
-    obj, \
-    if (PSScavenge::should_scavenge(p)) { \
-      pm->claim_or_forward_depth(p); \
-    }, \
-    assert_nothing )
-}

and has now been moved to:
http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/gc_implementation/parallelScavenge/psPromotionManager.cpp.udiff.html

+void InstanceKlass::oop_ps_push_contents(oop obj, PSPromotionManager* pm) {
+  PushContentsClosure cl(pm);
+  oop_oop_iterate_oop_maps_reverse<true>(obj, &cl);
+}

where the do_oop function is implemented as:
+
+  template <typename T> void do_oop_nv(T* p) {
+    if (PSScavenge::should_scavenge(p)) {
+      _pm->claim_or_forward_depth(p);
+    }

 From the same file, it can be seen how that the implementation to 
follow the references in the mirrors are using the same closure:

+void InstanceMirrorKlass::oop_ps_push_contents(oop obj, 
PSPromotionManager* pm) {
...
+  InstanceKlass::oop_ps_push_contents(obj, pm);
+
+  PushContentsClosure cl(pm);
+  oop_oop_iterate_statics<true>(obj, &cl);
+}

As can be seen above, the functions are still members of the different 
Klasses, but only the declaration is put in the klass.hpp files. The 
actual implementation is put in the GC files. This helps decoupling the 
different GCs and the Klasses. We could move the functions over to a GC 
specific "visitor" and have a generic "accept" function in the Klasses, 
but that approach would require two virtual calls, while the current 
implementation only needs one.

In the future we might want to remove these functions from the *Klasses 
and reuse the already existing code in the oop_oop_iterate framework.

If we take the InstanceMirrorKlass::oop_ps_push_contents function above 
as an example, it could be implemented with 
InstanceMirrorKlass::oop_oop_iterate, since they share the same structure:

   54 template <bool nv, class OopClosureType>
   55 int InstanceMirrorKlass::oop_oop_iterate(oop obj, OopClosureType* 
closure) {
   56   InstanceKlass::oop_oop_iterate<nv>(obj, closure);
   57
   58   if (Devirtualizer<nv>::do_metadata(closure)) {
   59     Klass* klass = java_lang_Class::as_Klass(obj);
   60     // We'll get NULL for primitive mirrors.
   61     if (klass != NULL) {
   62       Devirtualizer<nv>::do_klass(closure, klass);
   63     }
   64   }
   65
   66   oop_oop_iterate_statics<nv>(obj, closure);
   67
   68   return oop_size(obj);
   69 }

Parallel Scavenge doesn't visit the klass pointers and do_metadata 
returns false, so that code path will be eliminated by the compiler. We 
would have to do something about the return oop_size(obj), since we 
don't want to do that unnecessarily. To change the GC object visitors to 
entirely use the oop_oop_iterator framework is out of scope for this patch.

The object visitor functions were renamed and moved as follows:
  oop_follow_contents(opp) -> oop_ms_follow_contents(oop) in markSweep.cpp
  oop_adjust_pointers(oop) -> oop_ms_adjust_pointers(oop) in markSweep.cpp
  oop_follow_contents(oop, ParCompactionManager*) -> 
oop_pc_follow_contents(...) in psCompactionManager.cpp
  oop_update_pointers(oop) -> oop_pc_update_pointers(oop) in 
psParallelCompact.cpp
  oop_push_contents(oop, PSPromotionManager*) -> 
oop_ps_push_contents(...) in psPromotionManager.cpp


- The oop iterate  macros used to take an assert parameter to be applied 
to oop* that were visited. By default, the check was 
assert_is_in_closed_subset, but MS, PS and PC provided their own 
asserts. ExtendedOopClosure has been handed the task to provide the 
default verification, but also a way to turn it off for individual 
closures, so that they can provide their own asserts. See: 
ExtendedOopClosure::verify() and 
ExtendedOopClosure::should_verify_oops() and how  the Devirtualizer 
dispatch class calls the verification code:

http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/memory/iterator.inline.hpp.udiff.html

+template <class OopClosureType, typename T>
+inline void Devirtualizer<true>::do_oop(OopClosureType* closure, T* p) {
+  debug_only(closure->verify(p));
+  closure->do_oop_nv(p);
+}


- Moved PSParallelCompact::MarkAndPushClosure::do_oop, 
PSParallelCompact::AdjustPointerClosure::do_oop, mark_and_push, adjust 
pointer, and follow_klass to psParallelCompact.inline.hpp

http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/gc_implementation/parallelScavenge/psParallelCompact.hpp.udiff.html


- Add seemingly incorrect includes between GCs. This is needed since we 
currently have no separation between GCs when we generate the 
oop_since_save_marks_iterate functions. See:
http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/memory/defNewGeneration.cpp.udiff.html

and the include of ParNew specific closures:
   51 #if INCLUDE_ALL_GCS
   52 #include "gc_implementation/parNew/parOopClosures.hpp"
   53 #endif

which is needed to be able to generate:
  856 ALL_SINCE_SAVE_MARKS_CLOSURES(DefNew_SINCE_SAVE_MARKS_DEFN)

This should be changed in a separate patch, so that DefNew only 
generates oop_since_save_marks_iterate that takes DefNew specfic closures.


The initial performance runs showed a slight increase of the GC times on 
some benchmarks on Solaris and Windows. The reason for this increase was 
that the these compilers didn't inline as much as the hand-inlined 
macros used to do. To remedy this I've increased the inlining in to ways:

- Turned on extra inlining for psPromotionManager.cpp when compiling 
with the Solaris Studio Compiler

http://cr.openjdk.java.net/~stefank/8075955/webrev.01/make/solaris/makefiles/product.make.udiff.html


- Added __forceinline to the InstanceKlass::oop_oop_iterate functions 
when compiling with the MS compiler.

http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/instanceKlass.inline.hpp.html


A previous version of this patch has been run through our testing, but 
I've recently done changes and extracted smaller patches, so I'll have 
to rerun all testing.

Any requests for extra testing to be done?

Thanks,
StefanK