RFR: 8075955: Replace the macro based implementation of oop_oop_iterate with a template based solution
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Mar 27 19:17:15 UTC 2015
Stefan,
This is a huge change and I've never pretended to know how these macros
work. I won't pretend I can read these templates quickly either. From
my runtime perspective, the change looks fine. Actually, they look
great. I have a couple minor comments but nothing deep.
http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/gc_implementation/parallelScavenge/psCompactionManager.cpp.udiff.html
+void InstanceKlass::oop_pc_follow_contents(oop obj, ParCompactionManager* cm) {
It seems like these functions never belonged in InstanceKlass, etc and
with your change, can you make them actually owned by the GC function
they call? Like this one seems that it should be owned by
PSParallelCompact. Or do they all have to have the same name?
http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/oops/objArrayKlass.hpp.udiff.html
+ // Iterate over oop elements with indices within [start, end), and metadata.
+ // Iterate over oop elements within [start, end), and metadata.
These have the wrong sort of closed bracket (messes up 'vi') - should be ']'
That's all.
Coleen
On 3/26/15, 12:35 PM, 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