RFR (S): 8043243: convert SCAN_AND_FORWARD, SCAN_AND_ADJUST_POINTERS, SCAN_AND_COMPACT macros to methods
Marcus Larsson
marcus.larsson at oracle.com
Fri Oct 24 15:00:41 UTC 2014
Hi Kim,
On 23/10/14 21:57, Kim Barrett wrote:
> 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.
>
Good catch. Saw this definition when I started on the bug but had
forgotten about it now. I removed the definition and made the function
pure virtual in the Space class instead.
> ==============================================================================
> 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.]
>
Added some comments like these for the CompactibleSpace class, which
will hopefully better explain the situation. Let me know if this is good
or if you want to add/change anything.
> 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.
>
This seems to me like the most straight forward way of solving this. It
also makes more sense that the verification uses the logic dictated by
the actual space class, rather than the sometimes simplified
scanned_block_is_obj check. Removed the
CompactibleSpace::scanned_block_is_obj and changed the assert.
> 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.]
Seeing how the auxiliary functions are meant exclusively for the
scan_and_* functions it makes more sense not to expose them. Updated the
patch to make these functions private, with the necessary friend
declarations for scan_and_* to still be able to call them.
>
> - 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 ...
>
> ==============================================================================
>
New Webrev:
http://cr.openjdk.java.net/~mlarsson/8043243/webrev.01/
Incremental:
http://cr.openjdk.java.net/~mlarsson/8043243/webrev.00-01/
Thanks,
Marcus
More information about the hotspot-gc-dev
mailing list