RFR: 8075955: Replace the macro based implementation of oop_oop_iterate with a template based solution

[Kim Barrett]

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

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<narrowOop>(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<T>(obj);
void oop_follow_contents_specialized(oop obj);

public:

// use this when UseCompressedOops is known.
// OopRep is either narrowOop or oop.
template<typename OopRep>
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<narrowOop>(oop obj) {
  oop_follow_contents_narrow(obj);
}

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

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