RFR: 8075955: Replace the macro based implementation of oop_oop_iterate with a template based solution (original) (raw)

Stefan Karlsson stefan.karlsson at oracle.com
Wed Apr 1 13:11:47 UTC 2015

On 2015-04-01 14:58, Bengt Rutisson wrote:

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.

Thanks, Bengt!

StefanK

Bengt On 2015-03-26 17:35, Stefan Karlsson wrote: Hi,

Please review this patch to replace the macro based implementation of oopoopiterate 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 oopoopiterate 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 oopoopiterate 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::oopoopiteratenv(...) call. The macros just calls the new template functions: 191 #define InstanceKlassOOPOOPITERATEDEFN(OopClosureType, _nvsuffix) _ 192 int InstanceKlass::oopoopiterate##nvsuffix(oop obj, _OopClosureType* closure) { _ 193 return oopoopiterate<nvstobool(nvsuffix)>(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 oopoopiterate 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/gcimplementation/g1/g1OopClosures.cpp.udiff.html CMS: http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/gcimplementation/concurrentMarkSweep/cmsOopClosures.cpp.html ParNew: http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/gcimplementation/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->oopiterate(&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->pushcontents(...), which calls Klass::ooppushcontents(...), 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 oopoopiterate 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::ooppushcontents(PSPromotionManager* pm, oop obj) { _- InstanceKlassOOPMAPREVERSEITERATE( _ _- obj, _ _- if (PSScavenge::shouldscavenge(p)) { _ _- pm->claimorforwarddepth(p); _ _- }, _ - assertnothing ) -} and has now been moved to: http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/gcimplementation/parallelScavenge/psPromotionManager.cpp.udiff.html +void InstanceKlass::ooppspushcontents(oop obj, PSPromotionManager* pm) { + PushContentsClosure cl(pm); + oopoopiterateoopmapsreverse(obj, &cl); +} where the dooop function is implemented as: + + template void dooopnv(T* p) { + if (PSScavenge::shouldscavenge(p)) { + pm->claimorforwarddepth(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::ooppspushcontents(oop obj, PSPromotionManager* pm) { ... + InstanceKlass::ooppspushcontents(obj, pm); + + PushContentsClosure cl(pm); + oopoopiteratestatics(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 oopoopiterate framework. If we take the InstanceMirrorKlass::ooppspushcontents function above as an example, it could be implemented with InstanceMirrorKlass::oopoopiterate, since they share the same structure: 54 template <bool nv, class OopClosureType> 55 int InstanceMirrorKlass::oopoopiterate(oop obj, OopClosureType* closure) { 56 InstanceKlass::oopoopiterate(obj, closure); 57 58 if (Devirtualizer::dometadata(closure)) { 59 Klass* klass = javalangClass::asKlass(obj); 60 // We'll get NULL for primitive mirrors. 61 if (klass != NULL) { 62 Devirtualizer::doklass(closure, klass); 63 } 64 } 65 66 oopoopiteratestatics(obj, closure); 67 68 return oopsize(obj); 69 } Parallel Scavenge doesn't visit the klass pointers and dometadata returns false, so that code path will be eliminated by the compiler. We would have to do something about the return oopsize(obj), since we don't want to do that unnecessarily. To change the GC object visitors to entirely use the oopoopiterator framework is out of scope for this patch. The object visitor functions were renamed and moved as follows: oopfollowcontents(opp) -> oopmsfollowcontents(oop) in markSweep.cpp oopadjustpointers(oop) -> oopmsadjustpointers(oop) in markSweep.cpp oopfollowcontents(oop, ParCompactionManager*) -> ooppcfollowcontents(...) in psCompactionManager.cpp oopupdatepointers(oop) -> ooppcupdatepointers(oop) in psParallelCompact.cpp ooppushcontents(oop, PSPromotionManager*) -> ooppspushcontents(...) 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 assertisinclosedsubset, 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::shouldverifyoops() 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::dooop(OopClosureType* closure, T* p) { + debugonly(closure->verify(p)); + closure->dooopnv(p); +} - Moved PSParallelCompact::MarkAndPushClosure::dooop, PSParallelCompact::AdjustPointerClosure::dooop, markandpush, adjust pointer, and followklass to psParallelCompact.inline.hpp http://cr.openjdk.java.net/~stefank/8075955/webrev.01/src/share/vm/gcimplementation/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 oopsincesavemarksiterate 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 INCLUDEALLGCS 52 #include "gcimplementation/parNew/parOopClosures.hpp" 53 #endif which is needed to be able to generate: 856 ALLSINCESAVEMARKSCLOSURES(DefNewSINCESAVEMARKSDEFN) This should be changed in a separate patch, so that DefNew only generates oopsincesavemarksiterate 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::oopoopiterate 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