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

[Marcus Larsson]

Hi Kim,

I appreciate your feedback on this. Some comments are inline.


On 09/19/2014 08:52 PM, Kim Barrett wrote:
> On Sep 8, 2014, at 7:17 AM, Marcus Larsson <marcus.larsson at oracle.com> wrote:
>> New webrev:
>> http://cr.openjdk.java.net/~brutisso/webrev-8043243v2/
> Sorry for the late review; I was on vacation and only now catching up
> on this.
>
> This change replaces some macros with function templates.  These
> function templates take a "space" argument and require that it support
> a specific protocol.  Actually, there seem to be two protocols, one a
> subset of the other:
>
>   // base protocol
>   HeapWord* scan_limit(space*)
>   bool block_is_obj(space*, const HeapWord*)
>   size_t block_size(space*, const HeapWord*)
>
>   // addtional protocol
>   size_t adjust_obj_size(space*, size_t)
>   size_t obj_size(space*, const HeapWord*)
>
> [Not sure why space arguments are not const in most or all cases.]

This was also my first thought, but after digging deeper I found that 
there's really only a single protocol.
(Either that, or you could say there are three protocols, one for each 
template function.)
Each subclass of CompactibleSpace seems to make use of all of the 
template functions,
sometimes via CompactibleSpace::adjust_pointers/compact, and hence 
require all of the above functions
(both protocols), although they sometimes also override these functions 
when necessary.

> The "usual" way to do that is with virtual functions declared in the
> base class and implemented in the derived classes.  I'm going to
> ignore that approach, as I suspect it was (probably correctly) deemed
> too expensive in performace for these operations that look to be
> fairly light weight and probably performance critical.  I also put
> "usual" in quotes, since in some communities or contexts that would
> not at all be the preferred approach.
>
> A point of nomenclature: the proposed code uses the term "proxy",
> where I think the more usual terminology (per Gang-of-Four) would be
> "adaptor" (or "adapter" - the C++ community seems to prefer the -or
> spelling, GoF uses -er).
>
> One thing that is missing is any definition of the "concepts" required
> by these function templates.  Presently that's only implicitly encoded
> in the calls made by those functions, and by the definitions of the
> new "proxy" classes.  There really should be at least documentation
> associated with the functions describing their requirements.  Code to
> check the arguments satisfy those requirements would be better, but I
> don't think hotspot has any infrastructure to help with that.

I added some comments trying to document the requirements on the 
SpaceTypes for these functions.

>
> The function templates are declared as ordinary member functions
> rather than static member functions, with the requirement that they
> will be called with "this" as the first (space) argument, e.g.
>   "this == space"
> is a requirement for these functions.  That should be asserted, or
> these functions should be static member functions.  I don't know if
> the latter will work without further changes; there might be uses of
> the implicit this in the current implementations of these functions,
> which may argue that the presently implicit "concept" definitions are
> incomplete.

Changed the template functions to be static instead, like they probably 
should have been from the beginning.
The previous solution had some functions called via the implicit "this", 
while others were called using the proxy.

>
> The proposed adaptation mechanism is to provide an auxiliary class
> providing static functions on the space value that forward to related
> functions in that object.
>
> Alternatives that come to mind that I like better include:
>
> - Add the needed functions to the associated classes, rather than
> providing auxiliary forwarding classes.  Note that this doesn't
> involve virtual functions; I'm suggesting static polymorphism here,
> not runtime polymorphism.  These should be ordinary member functions
> invoked as space->fn().  [They could instead be static member
> functions taking space as an argument, but then they would need to be
> called as SpaceType::fn(space), which doesn't seem like an
> improvement.]  This is, of course, an intrusive approach, but the scan
> function templates are in the API of a base class, so providing
> support directly in the derived classes is reasonable.

I think this is a good idea, and modified the changeset to use this 
alternative rather than the auxiliary proxy/adapter classes.
To avoid name collisions, the block_is_obj and block_size functions used 
during scanning were named scanned_block_is_obj
and scanned_block_size.

To allow non-virtual calls of block_size for HeapRegions I moved the 
prepare_for_compaction from G1OffsetTableContigSpace
to the HeapRegion class, and implemented the required functions in this 
class instead.

>
> - Add free functions with overloads for the relevant classes.  Lack of
> namespace usage in hotspot might make this less attractive.  Also,
> there aren't good primary / default definitions available for most or
> all of these operations.
>
> Assuming the auxiliary class approach is retained, the current
> implementation has the adaptor (called proxy) type as a template
> parameter that the caller must explicitly supply.  A better approach
> would be to compute the adaptor type from the space type in the body
> of the function templates.
>
> The adaptor type could be added directly to the relevant space types
> as a public typedef, e.g. in each class add
>
>   typedef ADAPTOR_TYPE ScanAdaptor;
>
> A non-intrusive approach would be to provide a metafunction for
> mapping from space type to adaptor type, e.g.
>
> - Add to CompactibleSpace class
>
>   template<typename T> struct ScanAdaptor;
>
> - For each space class that requires scan adaptation, define
>
> template<>
> struct CompactibleSpace::ScanAdaptor<SPACE_TYPE> {
>   typedef ADAPTOR_TYPE type;
> };
>
> Then, in each of the scan templates, eliminate the SpaceProxy template
> parameter and add one of the following typedefs to the body, as
> appropriate for the computation approach:
>
>   typedef typename SpaceType::ScanAdaptor SpaceProxy;
>
> or
>
>   typedef typename CompactibleSpace::ScanAdaptor<SpaceType>::type SpaceProxy;
>
> [possibly renaming SpaceProxy]
>
> This eliminates the need for callers to explicitly specify the adaptor
> type in calls to the scan functions.
>
> There are other approaches to this whole problem, such as using CRTP
> (the Curiously Recurring Template Pattern) but that would be
> substantially further from the presently proposed changes, and I think
> not necessarily better in this case, so I'm not going to discuss that
> further.
>
> That's a lot of commentary, with several possible options, so I'll be
> specific about what I currently think should be done.
>
> - Document the requirements of the new function templates on their
>   space argument, e.g. what operations must SpaceType provide.  [Some
>   documentation about what these functions do might be nice too.]
>
> - Change the new function templates to be static member functions
>   rather than ordinary member functions, or if that is too onerous
>   then at least assert this == space.
>
> - Add the needed operations directly to the relevant classes, as
>   ordinary member functions, and eliminate the auxiliary classes.
>   Alternatively, if the auxiliary class approach is preferred by
>   others, then compute that class from the space class by one of the
>   methods suggested above.
>

As a side note I was unable to compile this with disabled precompiled 
headers, so the new changeset also has some some new #includes to fix this.

New webrev:
http://cr.openjdk.java.net/~mlarsson/8043243/webrev.00/

Regards,
Marcus