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
Thu Apr 2 06:45:27 UTC 2015

On 2015-04-02 02:08, Kim Barrett wrote:

On Mar 26, 2015, at 12:35 PM, Stefan Karlsson <stefan.karlsson at oracle.com> 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 I haven't had time to do a careful line by line review, though I've looked at a lot of the code. So far I've mostly been looking at structural things and overall design. Below are a few comments. While I think we can do better, I think this is very much a step in the right direction.

I fully agree.

I don't think anything below is required for acceptance of this change set. I might have more comments later, but don't let that possibility hold you back.

Thanks. We can always change the code again.

------------------------------------------------------------------------------ I would like to have the definitions of the various closure classes and their dooopnv functions placed in their natural locations. I really hate that one might have a closure class that is used in one place in a .cpp, and one ends up putting the declaration and definition far away in some other .hpp and .inline.hpp files. I think this could be done within the proposed infrastructure, but don't think it should be part of this change set. (It might even have been possible previously.) The idea is that, rather than having files like g1OopClosure.hpp and g1OopClosure.inline.hpp, instead define the closure class near where it is used, and follow it's definition with an application of ALLKLASSOOPOOPITERATEDEFN (if I've remembered the right macro).

Yes. I intentionally left the closure implementation where they are located for this patch. One of the motivation for the rewrite is to be able to implement what you suggest.

I would also like to get rid of all the OOP_OOP_ITERATE_DEFN macros.

------------------------------------------------------------------------------ I think that for "specialized" closure classes we shouldn't even provide or generate code for the "generic" case that we don't ever want to have used. Ideally we want a compiler error if we unintentionally attempt to apply a specialized closure in an unspecialized context. And we should work hard to eliminate the intentional cases. For those cases where a specialized closure gets passed through some API that loses the specialized information, it would be much better if we can recover most of it later, rather than just falling into the generic iterate calling virtual dooop for each subobject. [What I'm thinking of for the above is the visitor-like pattern you alluded to, in conjunction with the specialization framework. So a specialized closure class would implement the visitor as calling oopiterate with itself, with the specialization mechanism transforming that into a specialized call. So a "good" oopiterate has only one virtual call for specialized closures, while a type-erased oopiterate for specialized closures takes two virtual function calls, the first to the closure's visitor to recover the closure's type.] I think these can be accomplished with a better template-based dispatching infrastructure for oopiterate and friends. [I think I know how to accomplish this, but haven't had time to get any of it written down in a form that is coherent even to me.]

Sounds good to me. There are a few places where the virtual do_oop functions are used outside of the oop_iterate framework, so we would have to cater for those as well. For example, see the usage of the AdjustPointersClosure in PSParallelCompact::adjust_roots().

------------------------------------------------------------------------------ I think a lot of dispatches on UseCompressedOops can be eliminated by doing things like the following. (Warning: I haven't made any attempt to actually try this in real code. This is only a sketch so far.) This also foreshadows some of what I'm thinking for oopiterate dispatching. But it's a little easier without the extra complexity of dealing with the closure argument whose exact type we want to propogate through some API layers. Change class Klass: remove virtual oopfollowcontents. remove namespace-scope oopfollowcontentsspecialized, instead making them (non-virtual protected) ordinary member function templates for the corresponding classes. Replace with protected: // each derived class implements these with the "specialized" code, // e.g. call oopfollowcontentsspecialized(obj). virtual void oopfollowcontentsnarrow(oop obj) = 0; virtual void oopfollowcontentswide(oop obj) = 0; // each derived class with a specialized implementation of // oopfollowcontents provides protected. a base class // implementation can be called from a derived class, e.g. at the end // of InstanceKlassRef::oopfollowcontentsspecialized(oop obj), // replace // klass->InstanceKlass::oopfollowcontents(obj); // with // InstanceKlass::oopfollowcontentsspecialized(obj); void oopfollowcontentsspecialized(oop obj); public: // use this when UseCompressedOops is known. // OopRep is either narrowOop or oop. template void oopfollowcontents(oop obj); // use this when OopRep is not known, to dispatch on UseCompressedOops void oopfollowcontents(oop obj); ---- definitions ---- template<> void Klass::oopfollowcontents(oop obj) { oopfollowcontentsnarrow(obj); } template<> void Klass::oopfollowcontents(oop obj) { oopfollowcontentswide(obj); } void Klass::oopfollowcontents(oop obj) { if (UseCompressedOops) { oopfollowcontents(obj); } else { oopfollowcontents(obj); } }

The implementations of oop_follow_contents_narrow and oop_follow_contents_wide are the same in all code I've changed. So, we would have to either code duplicate the implementation, or yet again have to dispatch to a specialized version.

void InstanceKlass::oop_follow_contents_wide(oop obj) { oop_follow_contents_specialized(obj); }

void InstanceKlass::oop_follow_contents_narrow(oop obj) { oop_follow_contents_specialized(obj); }

I see that this would be one way to get rid of the UseCompressedOops call when calling InstanceKlass::oop_follow_contents from one of the sub-classes. I'm not convinced that that is important for performance, or that the proposal makes the code any cleaner. What would the motivation for your suggested change be?

Thanks for looking at the patch! StefanK

More information about the hotspot-dev mailing list