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

[Marcus Larsson]

Hi,

I think that the CRTP-based approach should be done as a separate task 
if this is something we want to do. The current solution solves the 
problem of getting rid of the macros, in a pretty straight forward way, 
IMHO. If you really prefer the proxy/adapter solution, I could rework a 
cleaner version of it (without the previous implicit this usage). If 
not, I think the current solution should be used, possibly with 
additional comments to avoid future mistakes (for example further 
explaining that scanned_block_* methods are meant exclusively for 
scan_and_* functions).

Thanks,
Marcus

On 09/10/14 17:57, Erik Österlund wrote:
> 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 "\"
>>>
>