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

[Erik Helin]

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.

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