RFR: 8075955: Replace the macro based implementation of oop_oop_iterate with a template based solution
Bengt Rutisson
bengt.rutisson at oracle.com
Wed Apr 1 12:58:25 UTC 2015
Hi Stefan,
We've looked through previous versions of this patch in quite some
detail. I've browsed the latest webrev and as far as I can tell it looks
good.
Reviewed.
Bengt
On 2015-03-26 17:35, Stefan Karlsson wrote:
> 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
>
More information about the hotspot-dev
mailing list