Marcus Larsson marcus.larsson at oracle.com
Wed Oct 22 14:31:47 UTC 2014


On 22/10/14 00:29, Kim Barrett wrote:
> 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.

No problem. Sounds good!

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

The way I look at it is that if the behavior of the 
adjust_pointers/compact/prepare_for_compaction should be modified, the 
associated helper functions such as scanned_block_size must be 
explicitly modified to reflect this. The fact that scanned_block_size 
might call the block_size method of that particular class is nothing 
that should be expected or relied upon for subclasses of that type.

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

By separating the helper functions used in scan_and_* function templates 
from the virtual functions of the Space class, I don't think this change 
violates any expectations. The scanned_block_* functions are not 
necessarily equal to the block_* functions of the Space class, even 
though they might sometimes be implemented as such. This basically works 
the same way as before, only without the macros and with actual 
functions to be used during scanning instead.

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

Will do.


More information about the hotspot-gc-dev mailing list