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

[Stefan Karlsson]

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

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

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 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);
>    }
> }

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<oop>(obj);
}

void InstanceKlass::oop_follow_contents_narrow(oop obj) {
   oop_follow_contents_specialized<narrowOop>(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
>
>