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

Kim Barrett kim.barrett at oracle.com
Thu Apr 2 00:08:18 UTC 2015

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 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.

I would like to have the definitions of the various closure classes and their do_oop_nv 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 ALL_KLASS_OOP_OOP_ITERATE_DEFN (if I've remembered the right macro).

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 do_oop 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 oop_iterate with itself, with the specialization mechanism transforming that into a specialized call. So a "good" oop_iterate has only one virtual call for specialized closures, while a type-erased oop_iterate 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 oop_iterate 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.]

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 oop_iterate 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 oop_follow_contents. remove namespace-scope oop_follow_contents_specialized, 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 oop_follow_contents_specialized(obj). virtual void oop_follow_contents_narrow(oop obj) = 0; virtual void oop_follow_contents_wide(oop obj) = 0;

// each derived class with a specialized implementation of // oop_follow_contents provides protected. a base class // implementation can be called from a derived class, e.g. at the end // of InstanceKlassRef::oop_follow_contents_specialized(oop obj), // replace
// klass->InstanceKlass::oop_follow_contents(obj); // with
// InstanceKlass::oop_follow_contents_specialized(obj); void oop_follow_contents_specialized(oop obj);

public:

// use this when UseCompressedOops is known. // OopRep is either narrowOop or oop. template void oop_follow_contents(oop obj);

// use this when OopRep is not known, to dispatch on UseCompressedOops void oop_follow_contents(oop obj);

---- definitions ---- template<> void Klass::oop_follow_contents(oop obj) { oop_follow_contents_narrow(obj); }

template<> void Klass::oop_follow_contents(oop obj) { oop_follow_contents_wide(obj); }

void Klass::oop_follow_contents(oop obj) { if (UseCompressedOops) { oop_follow_contents(obj); } else { oop_follow_contents(obj); } }

More information about the hotspot-dev mailing list