RFR: 8075955: Replace the macro based implementation of oop_oop_iterate with a template based solution
Stefan Karlsson
stefan.karlsson at oracle.com
Mon Mar 30 07:38:27 UTC 2015
Hi Coleen,
On 2015-03-27 20:17, Coleen Phillimore wrote:
>
> 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.
Thanks for looking at this patch.
> 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?
I agree that we want to get rid of these functions from the *Klasses,
but I don't see a small, incremental change that I could do to solve it.
Two alternatives that I tried to explain further down in the mail:
1) Reuse the oop_oop_iterate framework. That will be quite a large
change by itself, but hopefully doable.
2) Make a common object visitor function, say
Klass::oop_accept(ObjectVisitor* visitor), and then have the GC code do:
PCFollowContents follow_contents(cm);
obj->klass()->oop_accept(&follow_contents);
The drawback of that solution is that we get two virtual calls: The
first is when calling Klass::oop_accept to get "inside" the correct
sub-Klass, and the second is the call to ObjectVisitor::visit(this /*
the sub-Klass */) from the dispatched sub-Klass.
Getting rid of the second virtual call is exactly what we accomplish by
having the oop_pc_follow_contents in the *Klasses.
>
> 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 ']'
I'm not sure I understand what you mean. The [ bracket means inclusive,
while the ) bracket is supposed to mean exclusive. Just like what's
described in:
http://en.wikipedia.org/wiki/Bracket_(mathematics)#Intervals
>
> That's all.
Thanks!
StefanK
> 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