RFR: 8075955: Replace the macro based implementation of oop_oop_iterate with a template based solution
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Mar 30 23:18:55 UTC 2015
On 3/30/15, 3:38 AM, Stefan Karlsson wrote:
> 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.
This was really just a comment and question. I don't think you should
change this. It's a big enough change!
>
>>
>> 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
Ok. Sure, it just looked odd to me and my non-mathematical editor.
Coleen
>
>>
>> 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