RFR (S): 8043243: convert SCAN_AND_FORWARD, SCAN_AND_ADJUST_POINTERS, SCAN_AND_COMPACT macros to methods

[Erik Österlund]

Hi,

On 09 Oct 2014, at 17:08, Erik Helin <erik.helin at oracle.com> wrote:

> On 2014-10-09 01:36, Kim Barrett wrote:
>> On Sep 25, 2014, at 6:28 AM, Marcus Larsson <marcus.larsson at oracle.com> wrote:
>>> 
>>> New webrev:
>>> http://cr.openjdk.java.net/~mlarsson/8043243/webrev.00/
>> 
>> I still haven't been able to do a really thorough review, but one
>> thing stood out for me:
>> 
>> ==============================================================================
>> compactibleFreeListSpace.hpp
>> scanned_block_is_obj()
>> scanned_block_size()
>> Both call CompactibleFreeList::block_xxx() to avoid virtual function
>> call overhead.
>> 
>> Similarly in g1/heapRegion.hpp
>> scanned_block_size()
>> 
>> This "devirtualization" is an unsafe abstraction violation, since
>> there is nothing that would prevent the wrong function from being
>> called if a derived class were to override those directly called
>> implementations.  This devirtualization is also not present in the old
>> code that is being replaced.  [Note: The C++11 "final" specifier could
>> be used to make the devirtualization safe (by preventing any such
>> derived class overrides), but in that case the explicit
>> devirtualization wouldn't be necessary because the compiler could (and
>> should) infer it.]
>> 
>> If elimination of virtual function call overhead is a goal, then I
>> think a CRTP-based approach should probably be used instead.  I
>> suspect that's a pretty large underdaking though.
> 
> Correct me if I'm wrong, but as I understand the above paragraph, you are worried about someone subclassing CompactibleFreeListSpace, say FooCompactibleFreeListSpace, cast a FooCompactibleFreeListSpace* to a CompactibleFreeListSpace* and then become surprised when CompactibleFreeListSpace::scanned_block_is_obj is called on the CompactibleFreeListSpace* instead of FooCompactibleFreeListSpace::scanned_block_is_obj?
> 
> I don't think the above is likely to become an issue, I suspect that someone doing the above would quite soon find out that the wrong code was being called. IMO, I prefer the current implementation (or the one using the proxy), since a CRTP [0] based approach might be harder to understand for people not familiar with that pattern.

Couldn't help but have some opinions here since I already implemented these exact macros with a CRTP-solution and modular closures (and hence immune to such overrides of the scanned_block_is_obj member function etc.).

I'm of the opposite opinion that CRTP really is not that hard and that if people don't know it, they should probably learn it, so +1 vote for me if it counts even if I'm very opinionated and love templates haha. ;)

We can also use SFINAE templates to protect against unexpected overrides of methods such as the ones you mentioned here (even if I agree it's not a big problem).

Say we have some CRTP class Abstract<T>, Base<T>, Derived<T> where Base derives from Abstract and C derives from B. If Abstract has a pure virtual member function m() and both Base<T>  only Derived<T> define concrete implementations. An additional m_dispatch() member function is added to Abstract<T>

We can then define m_dispatch like this:

typename enable_if<has_member_function_m<T, void (T::*)()>::value, void>::type 
m_dispatch() { static_cast<T*>(this)->T::m(); }	// The CRTP type T declares its own T::m()

typename enable_if<!has_member_function_m<T, void (T::*)()>::value, void>::type
m_dispatch() { static_cast<T*>(this)->m(); }	// The CRTP type T does not declare its own T::m() but a parent might have it - use virtual call

(disclaimer: this should come with a template specialization for the virtual class so it always makes virtual calls too)

Now, as long as new types like Derived<T> are upcasted to a non-concrete class (Abstract<T> in my example) the template will detect any base classes automatically when it needs to make a virtual call and when it can remove the virtualization. Or simply generate a compiler error instead if that's the wanted behaviour.

I'm not saying templates are the silver bullet to all problems in life, but I'm convinced it solves the issues in this discussion. :)

There's my 50 cents on this...

/Erik

> Thanks,
> Erik
> 
> [0]: http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern
> 
>> I also think that a CRTP-based approach could to be tackled as a
>> separate step later, whilst still replacing the macros with function
>> templates now.  I think, though, that if that's the plan then my
>> previous suggestion of eliminating the adapter classes may have been a
>> mistake, as I suspect it might make the CRTP revision more
>> complicated.  Sorry for any whiplash.
>> 
>> ==============================================================================
>> space.inline.hpp
>> line 97: stray trailing "\"
>>