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

[Kim Barrett]

On Oct 8, 2014, at 7:36 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> I still haven't been able to do a really thorough review ...

Sorry about taking so long to get back to this.  I decided to put it
aside for a while after noticing the sinus infection &etc was making
me even more stupid than I'd previously realized.

I've resumed working on the review, and will try to finish this
evening or tomorrow.

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

I still think that's true.

> This devirtualization is also not present in the old
> code that is being replaced.

And that is quite wrong.  That devirtualization is actually much of
the point of the old macros.  Like I said, I was being stupid.

> 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. 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 still think CRTP is the right way to solve this, but also still
think that can be tackled as a later step.

The removal of some space types by Bengt's recent changes to remove
iCMS might even simplify that process a bit.

On Oct 9, 2014, at 11:08 AM, Erik Helin <erik.helin at oracle.com> wrote:
> 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?

Not exactly.  My issue is that the scan_and_* function templates are
being called in base class member functions, where the information
about the derived class has already been discarded (implicitly cast
away).  As a result, in order to decide whether the code is correct,
one needs to go look at all the derived classes, figure out where they
get the operations in question, and determine whether that matches
where the operations will be obtained from in the scan_and_* call
sites.  Similar issues arise when making changes.

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

Code readability and maintainability are important, and what's going
on here is pretty strongly contrary to that, as it is intentionally
violating usual expectations about inheritance.

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

CRTP is a widely used (hence the "Recurring") core technique for
static polymorphism in C++.  In my experience it's pretty hard to
avoid running across it in a non-trivial C++ code base these days.  It
looks syntactically odd if one has never seen it before, but it is
conceptually pretty simple.  So I disagree with the idea that CRTP
should be avoided on the basis of lack of familiarity by some.

On Oct 13, 2014, at 4:01 AM, Marcus Larsson <marcus.larsson at oracle.com> wrote:
> possibly with additional comments to avoid future mistakes (for
> example further explaining that scanned_block_* methods are meant
> exclusively for scan_and_* functions).

Yes to such comments.