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

Erik Österlund erik.osterlund at lnu.se
Tue Oct 21 23:21:08 UTC 2014


Hi,

If we indeed want CRTP (which I totally agree with) then maybe somebody finds it interesting to read my patch from when I fixed this with a light-weight CRTP and closure approach (was a while ago now). It is attached to this email and following is a short description of its main ideas for the curious. It's very straight forward!

1) The DerivedSpace is passed down from the entry member functions (prepare_for_compaction(), compact() etc) where the DerivedSpace type is known (rather than using the declared type of the Space as common with CRTP). The benefit of this approach is that a virtual call is made to the Space (of any declared type) to the entry member function. Once there, the DerivedSpace type is for sure known and determined safely (because of the virtual entry) and passed to the worker method for devirtualization - that way, there will be no errors if the declared Space type is more abstract than the actual type (which I think there have been expressed fears about in this conversation), while the virtual calls are avoided for the expensive parts (doing stuff with blocks and objects in the Space).

2) Instead of calling methods in Space directly from the worker member functions (calling block_is_obj etc) it uses modular closures as indirection that know about the DerivedSpace and could use its member functions (or anything else). Might be over-engineering but I thought it was pretty nice! (would prefer if the actual implementations were moved into the closures rather than indirecting non-virtual member functions of Space ideally)

3) ...and changed some horrible variable names to avoid nightmares about p and q.

Just throwing it in there if anyone wants to have a look and maybe gain some inspiration about the basic idea...
Hope I'm not cluttering the conversation with my template propaganda! :)

Cheers,
/Erik


On 22 Oct 2014, at 00:29, Kim Barrett <kim.barrett at oracle.com> 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.
>
>> 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.
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20141021/2b201dde/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stw_gc_crtp.diff.zip
Type: application/zip
Size: 7409 bytes
Desc: stw_gc_crtp.diff.zip
URL: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20141021/2b201dde/stw_gc_crtp.diff-0001.zip>


More information about the hotspot-gc-dev mailing list