RFR: 8162929: Enqueuing dirty cards into a single DCQS during GC does not scale

[Kim Barrett]

> On Jul 12, 2019, at 8:36 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> On Fri, 2019-07-05 at 00:50 -0400, Kim Barrett wrote:
>> Please review this refactoring of G1DirtyCardQueue[Set], splitting
>> into two sets of classes that each better support the purpose they
>> are used for.  […]
>> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8162929
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8162929/open.00/
>> 
> 
> some comments:
> 
> - pre-existing: I would prefer if PtrQueue::~PtrQueue would be virtual
> to avoid the risk of object slicing. There is no problem right now in
> the code afaics though, but we've been bitten by object slicing issues
> before, and it's imho okay to use a virtual destructor here.

There is no risk of object slicing. ~PtrQueue (and ~PtrQueueSet and
~SATBMarkQueueSet) are, by design, protected. The "usual" rule for
base class destructors is public and virtual (if support for deleting
through a base class pointer is desired), or protected and
non-virtual. (See, for example, http://www.gotw.ca/publications/mill18.htm,
Guideline #4.)

Also, making these destructors virtual would require PtrQueue and
PtrQueueSet to have an accessible operator delete.  The presence of a
virtual destructor induces the creation of a "deleting destructor",
which will call the operator delete found by lookup in the class.
That probably means deriving PtrQueue and PtrQueueSet from CHeapObj,
even though we don't actually want to heap allocate any of these
objects.  (In the original version of this change I made PtrQueueSet
derive from CHeapObj<mtGC>, and forgot to back that out before
starting the review.  What I was doing there is really more properly
part of JDK-8221361, and is probably the wrong approach anyway.  The
redirty card queue set should be local to a collection, perhaps part
of the evacuation info.)

The problem of unnecessarily inducing deleting destructors was also
discussed here:

http://mail.openjdk.java.net/pipermail/hotspot-dev/2012-December/007566.html
RFR (S): 8004845: Catch incorrect usage of new and delete during compile time for value objects and stack objects

AbstractGangTask was given a protected non-virtual destructor as part
of that change.  Unfortunately, that destructor was removed by
JDK-8087323, leaving it with the default (public) destructor.

Regretably, what seems like it should be the most relevant gcc warning
option (-Wnon-virtual-dtor) is overenthusiastic; -Wnon-virtual-dtor
warns about a class with virtual functions and an accessible
non-virtual destructor. That's broken. (Not just my opinion; I've seen
multiple discussions of this in various places.)  That warns about a
leaf derived class with a (necessarily) accessible non-virtual
destructor, because the compiler doesn't know it's a leaf
class. Warning if a class has a problematic base class would be fine.
But warning that some hypothetical (but nonexistent) derived class
could be sliced, because the compiler doesn't know that the leaf
derived class is actually a leaf, is just wrong. (This might be
improved using the C++11 final class specifier to let the compiler
recognize a leaf class.  But apparently not yet:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58140)

Unfortunately, even if -Wnon-virtual-dtor did the right thing, we'd
still get warnings from it because PtrQueue befriends VMStructs,
making its destructor accessible (though unused) from a non-derived
class. Drat!

The other relevant warning option (-Wdelete-non-virtual-dtor) has some
similar problems IMO (warning about hypothetical derived classes that
don't actually exist).

While I was looking at that, I noticed that while PtrQueue is
non-copyable, someone (likely me) forgot to do the same for
PtrQueueSet.

So I've made the following changes:
* Reverted PtrQueueSet derivation from CHeapObj.
* Made PtrQueueSet non-copyable.

but did not make ~PtrQueue or ~PtrQueueSet virtual.

> - I would prefer to add an "is_" prefix to the
> G1RedirtyCardsQueueSet::empty() member as this reads better.

empty() was only being used for assertions.  I've replaced it with
verify_empty().

> - maybe rename the local "QSet" to "LocalPtrQueueSet" to make it more
> self-describing. At least something a bit more descriptive if possible.

I renamed QSet to LocalQSet.

> - it's a bit unfortunate that on the way from the redirty qset to the
> global qsets (in G1DirtyCardQueueSet::merge_bufferlists) we can not
> easily keep a "tail" pointer and count, and then when merging the
> redirty qset with the dirty qset have to iterate over the BufferNodes
> to determine it.

The tail pointer and count are now carried all the way through to the
merge into the dirty card queue set, eliminating the iteration down
the (possibly very long) list in that merge.

> This might have some minor (seemingly unnecessary) impact on huge loads
> like we discussed internally when doing perf testing. Probably not
> worth the effort dealing with here, given that you may simply increase
> the buffer size there.
> I will do some cursory tests, with probably no detrimental outcome
> though.
> 
> Unfortunately there is no good way to just move the data between them
> other than the one actually implemented.

I think I found a good way to do that, enabled by the phased usage pattern.

> - in G1DirtyCardQueueSet::merge_bufferlists() I am not sure whether
> that loop used to determine the tail and the count is much favorable
> (e.g. easiert to read) than this due to endless loop + break
> "somewhere" within the loop body:

That loop is gone now.

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8162929/open.01/
incr: http://cr.openjdk.java.net/~kbarrett/8162929/open.01.inc/

Testing:
mach5 tier1-5