Devirtualize base::BindState to save ~1% of Chrome's binary size (~1MB)

base/callback_internal.h

Index: base/callback_internal.h
diff --git a/base/callback_internal.h b/base/callback_internal.h
index 8a5c4378bc1ccbfb5303727ecfd129dc49a42903..c83a1ba8d1ee3238676c6daea061f0b1b8f78fab 100644
--- a/base/callback_internal.h
+++ b/base/callback_internal.h
@@ -8,17 +8,19 @@
 #ifndef BASE_CALLBACK_INTERNAL_H_
 #define BASE_CALLBACK_INTERNAL_H_
 
-#include <stddef.h>
-
+#include "base/atomic_ref_count.h"
 #include "base/base_export.h"
-#include "base/memory/ref_counted.h"
-#include "base/memory/scoped_ptr.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"  // TODO(tapted): Remove.
+#include "base/memory/scoped_ptr.h"   // TODO(tapted): Remove.

[tapted, 2015/05/11 08:56:17]

ref_counted and scoped_ptr are no longer needed, but the diff explodes with IWYU
changes trying to remote them now. So I'll remove this in a follow-up.
(prettysure the ScopedVector forward declare below can go too).

+#include "base/template_util.h"
 
 template <typename T>
 class ScopedVector;
 
 namespace base {
 namespace internal {
+class CallbackBase;
 
 // BindStateBase is used to provide an opaque handle that the Callback
 // class can use to represent a function object with bound arguments.  It
@@ -26,21 +28,41 @@ namespace internal {
 // DoInvoke function to perform the function execution.  This allows
 // us to shield the Callback class from the types of the bound argument via
 // "type erasure."
-class BindStateBase : public RefCountedThreadSafe<BindStateBase> {
+// At the base level, the only task is to add reference counting data. Don't use
+// RefCountedThreadSafe since it requires the destructor to be a virtual method.
+// Creating a vtable for every BindState template instantiation results in a lot
+// of bloat. Its only task is to call the destructor which can be done with a
+// function pointer.
+class BindStateBase {
  protected:
-  friend class RefCountedThreadSafe<BindStateBase>;
-  virtual ~BindStateBase() {}
+  explicit BindStateBase(void (*destructor)(BindStateBase*))
+      : ref_count_(0), destructor_(destructor) {}
+  ~BindStateBase() = default;
+
+ private:
+  // Only CallbackBase ever calls AddRef/Release.
+  friend class CallbackBase;
+
+  void AddRef();
+  void Release();
+
+  AtomicRefCount ref_count_;
+
+  // Pointer to a function that will properly destroy |this|. Use a regular

[Nico, 2015/05/14 04:28:13]

nit: I think everything after the first sentence can be omitted. This doesn't
seem like something someone looking at this code would wonder about.

[tapted, 2015/05/14 05:06:54]

Done.

+  // function pointer, since their representation is often simpler/smaller than
+  // member function pointer types and it's not possible to take the address of
+  // a class destructor anyway.
+  void (*destructor_)(BindStateBase*);
+
+  DISALLOW_COPY_AND_ASSIGN(BindStateBase);
 };
 
 // Holds the Callback methods that don't require specialization to reduce
 // template bloat.
 class BASE_EXPORT CallbackBase {
  public:
-  CallbackBase(const CallbackBase& c);
-  CallbackBase& operator=(const CallbackBase& c);
-
   // Returns true if Callback is null (doesn't refer to anything).
-  bool is_null() const { return bind_state_.get() == NULL; }
+  bool is_null() const { return bind_state_ == nullptr; }
 
   // Returns the Callback into an uninitialized state.
   void Reset();
@@ -55,18 +77,23 @@ class BASE_EXPORT CallbackBase {
   // Returns true if this callback equals |other|. |other| may be null.
   bool Equals(const CallbackBase& other) const;
 
-  // Allow initializing of |bind_state_| via the constructor to avoid default
-  // initialization of the scoped_refptr.  We do not also initialize
-  // |polymorphic_invoke_| here because doing a normal assignment in the
-  // derived Callback templates makes for much nicer compiler errors.
+  // The default constructor is only used by Callback's default constructor.
+  CallbackBase();
+
+  // Allow initializing of |bind_state_| via the constructor. We do not also
+  // initialize |polymorphic_invoke_| here because doing a normal assignment in
+  // the derived Callback templates makes for much nicer compiler errors.
   explicit CallbackBase(BindStateBase* bind_state);
 
+  CallbackBase(const CallbackBase& c);
+  CallbackBase& operator=(const CallbackBase& c);
+
   // Force the destructor to be instantiated inside this translation unit so
   // that our subclasses will not get inlined versions.  Avoids more template
   // bloat.
   ~CallbackBase();
 
-  scoped_refptr<BindStateBase> bind_state_;

[Nico, 2015/05/14 04:28:13]

Shouldn't this still work (with another fried for this)? Templates are
structurally typed after all

[tapted, 2015/05/14 04:36:14]

The problem is that RefCounted<T>::Release has a static cast to <const T*> for
its delete. If BindStateBase was that T*, it would cast to itself and call the
wrong `delete`. And for that T* to be the "right" one (i.e. the fully-specified
BindState template), BindState would need to inherit from RefCountedThreadSafe
instead of BindStateBase, and that would result in a whole bunch of template
bloat.

(also I'd have to workaround the RefCountedThreadSafe style warning about not
having a virtual destructor :))

[tapted, 2015/05/14 05:06:54]

Done. The diff is tiny now \o/ (lots became irrelevant). I'm waiting on the
compile now, but if there's no size difference, you can see what I plan to land
in patchset 8.

The only sticky part was the DCHECK in the CallbackBase constructor. It needs
either an implementation of HasOneRef() on BindStateBase or a friend to accesss
the ref_count_ member. Went with a friend, since it felt like less work for the
compiler.

[tapted, 2015/05/14 05:19:58]

Locally, the framework got 9kB bigger, which doesn't seem worth optimizing for
(at least not in this CL).

If trybots stay happy I'll try landing this and keep a close eye on everything,
since the next couple of days will probably be hectic..

+  BindStateBase* bind_state_;
   InvokeFuncStorage polymorphic_invoke_;
 };