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

Kim Barrett kim.barrett at oracle.com
Thu Oct 23 19:57:46 UTC 2014


On Oct 21, 2014, at 6:29 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
> I've resumed working on the review, and will try to finish this
> evening or tomorrow.

Took a bit longer; it's a complicated set of changes.

==============================================================================
src/share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.hpp

 413   inline bool scanned_block_is_obj(const HeapWord* addr) const {
 414     return CompactibleFreeListSpace::block_is_obj(addr); // Avoid virtual call
 415   }
 416 
 417   inline size_t scanned_block_size(const HeapWord* addr) const {
 418     return CompactibleFreeListSpace::block_size(addr); // Avoid virtual call
 419   }

I think the devirtualization provided here is not present in the
original macro-based code.  The macro-based code just passes
unqualified block_is_obj and block_size to the use of SCAN_AND_FORWARD
in CompactibleFreeListSpace::prepare_for_compaction(), rather than
qualifying them.  There also isn't a macro override of those names in
the original code.

I think this is actually a bug in original macro-based code!  Nice fix!

There is a similar change for HeapRegion, where the new code
devirtualizes block_size, while the old code seems to be calling the
virtual function. [This required moving prepare_for_compaction with
associated call to scan_and_forward from G1OffsetTableContigSpace to
HeapRegion.]

==============================================================================
src/share/vm/memory/space.hpp
 240   // Mark-sweep-compact support: all spaces can update pointers to objects
 241   // moving as a part of compaction.
 242   virtual void adjust_pointers();

Is this definition still used or needed?  If it is, then there may be
other problems.

==============================================================================
I'm very close to saying "looks good other than the above".  However,
this bit 

src/share/vm/memory/space.hpp
[in class CompactibleSpace]
 431   inline bool scanned_block_is_obj(const HeapWord* addr) const {
 432     // Perform virtual call. This is currently not a problem since this
 433     // function is only used in an assert (from scan_and_adjust_pointers).
 434     return block_is_obj(addr);
 435   }

is bothering me.

This is a potential silent performance bug.  A derived class that
overrides block_is_obj() and prepare_for_compaction(), but forgets (or
misspells) scanned_block_is_obj() will quietly use virtual function
calls.

Also, I would like there to be a clear and simple theory (preferably
actually written out as a comment) regarding where various definitions
need to appear in the class hierarchy.  Something along the lines of

- The following are auxiliary functions used by the scan_and_xxx
function templates:

  scan_limit()
  scanned_block_is_obj()
  scanned_block_size()
  adjust_obj_size()
  obj_size()

- A class which overrides block_size() must provide a corresponding
  definition of scanned_block_size().

- A class which overrides block_is_obj() must provide a corresponding
  definition of scanned_block_is_obj().

- For each concrete Space class SC, the applicable definition for each
of 
    prepare_for_compaction()
    adjust_pointers()
    compact()
must be provided by a class for which the definitions of all of the
auxiliary functions [optional (see below): used by the associated
scan_and_xxx function template] are the same as for SC.

[optional]
Auxiliary function usage by the scan_and_xxx function templates is:
- scan_and_adjust_pointers() calls scanned_block_is_obj(), adjust_obj_size()
- scan_and_compact() calls obj_size()
- scan_and_forward() calls scan_limit(), scanned_block_is_obj(), scanned_block_size()

Yeah, sure, that's simple.  Well, it's simpler than any set of rules I
came up with that permitted the lines 431-435 that I find troubling.
[It's actually not too bad if the "optional" parts are left out,
losing some precision for a simpler rule set.]

I suspect that definition of CompactibleSpace::scanned_block_is_obj()
exists in order to avoid a little bit of source code duplication.  If
that definition were removed then ContiguousSpace and HeapRegion would
need to each override adjust_pointers().

I think we can do better here.

The smallest code change from the current webrev version is to
eliminate the dependency on scanned_block_is_obj() by
scan_and_adjust_pointers(), and just change the assert to explicitly
and directly call the virtual block_is_obj() function.  However, this
doesn't address the complexity of the usage rules that ought to be
documented, and need to be understood by anyone maintaining the code
in the future.

An alternative would be to just eliminate
CompactibleSpace::scanned_block_is_obj() and copy the
adjust_pointers() definition down to the two classes that need it,
e.g. HeapRegion and ContiguousSpace.  This gives up on the attempt to
avoid some source code duplication that I think is the rationale for
the current approach.  However, in the present hierarchy I think this
ends up being pretty much a wash for source code size.  [There's some
cost in generated code space, though some implementations may be able
to merge the perhaps identical generated code chunks.]  This lets us
use something like the above rule description, including the optional
parts.  It doesn't scale quite as well for maintenance though.

Another alternative is to add a CRTP-style helper class, and rearrange
things a bit to make use of it.  This is a larger change from the
current webrev offering, but has some nice properties.  Below is a
sketch of this idea, with some pieces to be filled in in what I hope
is obvious ways.

- The key part is the introduction of the ScanSupport class template.

- Eliminates the "optional" part of the above described usage rules,
since all of the scan_and_xxx operations are packaged in the
ScanSupport class.  That class is also where the usage rules are
located, but with respect to the use of the class; there's no mention
of the specific function templates.  [There may be some cost in
generated code space for this.]

- Hides the auxiliary functions from external access.  [The current
webrev exposes them publicly, though I'm not sure it needs to.]

- Easy to move specialization around in the class hierarchy, by just
moving derivations from ScanSupport.

The sketch:  [Hopefully there's enough detail here to be clear about
what I'm suggesting.]

// add to CompactibleSpace class, or add to global namespace with
// an appropriate prefix to the name:
template<typename Derived, typename Base = CompactibleSpace>
class ScanSupport;

template<typename Derived, typename Base>
class CompactibleSpace::ScanSupport : public Base {
public:

  // override virtual functions with implementations specialized for Derived.

  // *** benefit compared to current webrev: this is the only source
  // *** version of these. 
  virtual void prepare_for_compaction() {
    scan_and_forward(static_cast<Derived*>(this);
  }

  // ... etc ...

protected:

  ~ScanSupport() { }

  // Forward constructor calls to Base.  We presently support up to
  // two arguments; add more overloads if needed.
  // TODO: Replace with C++11 perfect forwarding variadic template, e.g.
  //   template<typename... T>
  //   ScanSupport(T&&... p) : Base(std::forward(p)...) { }
  // *** damage compared to current webrev: helper class needs to
  // *** provide this constructor forwarding.

  ScanSupport() : Base() { }

  template<typename T0>
  ScanSupport(const T0& a0) : Base(a0) { }

  template<typename T0, typename T1>
  ScanSupport(const T0& a0, const T1& a1) : Base(a0, a1) { }

private:

  // define the scan_and_xxx suite of functions.
  // *** benefit compared to current webrev: these function templates
  // *** are a completely private implementation detail of this class.

  template<typename T>
  static void scan_and_forward(T* space) {
    // ... body ...
  }

  // ... etc ...
};

// *** usage:

class ContiguousSpace
  : public CompactibleSpace::ScanSupport<ContiguousSpace>
{
  friend class CompactibleSpace::ScanSupport<ContiguousSpace>;

public:
  // virtual overrides as before
  virtual size_t block_size(const HeapWord*) const;
  // ... etc ...

private: // prevent external access, but ScanSupport friend can access
  // scan_and_xxx auxiliaries, as before
  size_t scanned_block_size(const HeapWord* addr) const {
    return ContiguousSpace::block_size(addr);
  }

  // ... etc ...

};

class HeapRegion
  : public CompactibleSpace::ScanSupport<HeapRegion, G1OffsetTableContigSpace>
{
  friend class CompactibleSpace::ScanSupport<HeapRegion, G1OffsetTableContigSpace>;

public:
  // virtual overrides as before
  virtual size_t block_size(const HeapWord*) const;
  // ... etc ...

private: // prevent external access, but ScanSupport friend can access
  // scan_and_xxx auxiliaries, as before
  size_t scanned_block_size(const HeapWord* addr) const {
    return HeapRegion::block_size(addr);
  }

  // ... etc ...
};

// *** update HeapRegion constructor for ScanSupport base class
HeapRegion::HeapRegion(
  uint hrm_index,
  G1BlockOffsetSharedArray* sharedArrayOffset,
  MemRegion mr)
  : CompactibleSpace::ScanSupport<
      HeapRegion,
      G1OffsetTableContigSpace>(sharedArrayOffset, mr),
    // ... etc ...

==============================================================================



More information about the hotspot-gc-dev mailing list