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

[Kim Barrett]

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

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.

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.

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.

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