RFR: 8209345: Merge SATBMarkQueueFilter into SATBMarkQueueSet

[Thomas Schatzl]

Hi,

On Tue, 2018-08-14 at 15:00 -0400, Kim Barrett wrote:
> > On Aug 14, 2018, at 4:54 AM, Thomas Schatzl <thomas.schatzl at oracle.
> > com> wrote:
> > 
> > On Mon, 2018-08-13 at 19:10 -0400, Kim Barrett wrote:
> > > > On Aug 13, 2018, at 6:09 AM, Thomas Schatzl <thomas.schatzl at ora
> > > > cle.
> > > > com> wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On Sun, 2018-08-12 at 22:08 -0400, Kim Barrett wrote:
> > > > > Please review this change that eliminates SATBMarkQueueFilter
> > > > > in
> > > > > favor
> > > > > of an extension to SATBMarkQueueSet.  […]
> > > > > 
> > > > > Webrev:
> > > > > http://cr.openjdk.java.net/~kbarrett/8209345/open.00/
> > > > > 
> > > > > Testing:
> > > > > Mach5 tier1-3, hs-tier4-5, in conjunction with JDK-8209346
> > > > > and
> > > > > JDK-
> > > > > 8209347.
> > > > > Locally tier1 and hotspot_gc without those other changes.
> > > > > 
> > > > 
> > > > - I think the SATBMarkQueueSet's destructor should be virtual,
> > > > given that there will be a second implementor with unknown
> > > > functionality (and in general I prefer to have virtual
> > > > destructors
> > > > as soon there is any virtual method). Also the
> > > > SATBBufferClosure's
> > > > one.
> > > 
> > > These destructors (and similarly for PtrQueue and PtrQueueSet)
> > > are
> > > protected, which is the other approach [1] to ensuring there will
> > > be
> > > no destructor slicing. Objects in these hierarchies are never
> > > destroyed via a base class pointer, so don't need a public base
> > > class
> > > destructor (which should indeed be virtual).
> > > 
> > > [1] "50. Make base class destructors public and virtual or
> > > protected
> > > and nonvirtual", C++ Coding Standards, Sutter & Alexandrescu.
> > > 
> > 
> >  this is true and good for this class in isolation, but the problem
> > will arise as soon as you will have descendents of this class which
> > make the destructor public.
> > 
> > At that point somebody needs to remember that the destructor needs
> > to
> > be declared as virtual, and people tend to forget. We've had a few
> > of
> > these issues already.
> > 
> > A general rule of "use a virtual destructor in the class that adds
> > virtual methods the first time" is simpler to remember and safer
> > than
> > everybody needing to remember (dev + reviewers) to look through the
> > class hierarchy whether the destructor is already virtual.
> > 
> > Now that's not completely fail-safe either (you can still forget),
> > but it seems a more uncommon failure mode than what these very
> > knowledgeable people suggest.
> > 
> > Thanks,
> >  Thomas
> 
> I disagree, but also think this RFR thread is the wrong place for
> this discussion.  I've pushed the change as-is, but we can continue
> the discussion elsewhere and the code can be modified if needed.

That's okay.

Thomas