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

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

Every call to base::Bind results in a new class declaration. Since
base::internal::BindState is a virtual class, this also results in a
bespoke vtable being defined. And that's a lot of vtables. Like 1MB
worth of vtables, or 1% of Chrome's binary size.

However, this vtable's sole purpose is for RefCountedThreadSafe::
Release(). Release() _does_ need to know precisely how to delete the
BindState, but it doesn't need a vtable to do it. All it needs is a
function pointer.

This CL de-virtualizes base::BindState and instead moves the vtable
pointer (which is merely an array holding the destructor) to a function
pointer that contains the destructor instead. Since BindState previously
contained a pointer-to-vtable the net memory effect of having the
function pointer instead should be offset.

And of course a 1MB binary size drop also means reduced memory
requirements for the shared text-segment pages.

Locally produced numbers for ChromiumFramework on Mac:
pre-strip:
 - before: 192,031kB
 - after: 185,800kB
post-strip:
 - before: 101,353kB
 - after: 100,353kB

BUG=486594
Committed: https://crrev.com/e7e804c7d491ea23cfbf5b57a9ec1f8fa78b44a7
Cr-Commit-Position: refs/heads/master@{#329818}

[tapted, 2015-05-08 09:20:06]

Patchset #3 (id:40001) has been deleted

[tapted, 2015-05-08 10:58:03]

Patchset #7 (id:140001) has been deleted

[tapted, 2015-05-08 12:49:29]

Patchset #4 (id:80001) has been deleted

[tapted, 2015-05-11 03:23:17]

Patchset #10 (id:240001) has been deleted

[tapted, 2015-05-11 03:23:26]

Patchset #7 (id:180001) has been deleted

[tapted, 2015-05-11 03:23:40]

Patchset #1 (id:1) has been deleted

[tapted, 2015-05-11 03:24:18]

Patchset #6 (id:200001) has been deleted

[tapted, 2015-05-11 05:26:48]

Patchset #7 (id:260001) has been deleted

[tapted, 2015-05-11 06:50:18]

Patchset #8 (id:300001) has been deleted

[tapted, 2015-05-11 07:08:03]

Patchset #1 (id:20001) has been deleted

[tapted, 2015-05-11 07:08:12]

Patchset #1 (id:60001) has been deleted

[tapted, 2015-05-11 07:08:23]

Patchset #1 (id:100001) has been deleted

[tapted, 2015-05-11 07:08:31]

Patchset #1 (id:120001) has been deleted

[tapted, 2015-05-11 07:17:43]

tapted@chromium.org changed reviewers:
+ thakis@chromium.org

[tapted, 2015-05-11 07:17:44]

Hi Nico, please take a look. I think this is worth doing. There's some
additional data using `-map` at http://crbug.com/486594

https://codereview.chromium.org/1129353003/diff/340001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/1129353003/diff/340001/base/callback_internal...
base/callback_internal.h:15: #include "base/memory/scoped_ptr.h"   //
TODO(tapted): Remove.
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. (pretty
sure the ScopedVector forward declare below can go too).

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

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal...
base/callback_internal.h:15: #include "base/memory/scoped_ptr.h"   //
TODO(tapted): Remove.
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).

[tapted, 2015-05-12 21:39:55]

Nico: ping?

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

This is super super cool.

Basically lgtm, but it'd be nice if we could keep using scoped_refptr (see
below)

(mikhail: Would this make sense for WTF::Function too?)

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal.h
File base/callback_internal.h (left):

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal...
base/callback_internal.h:69: scoped_refptr<BindStateBase> bind_state_;
Shouldn't this still work (with another fried for this)? Templates are
structurally typed after all

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal...
base/callback_internal.h:51: // Pointer to a function that will properly destroy
|this|. Use a regular
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 04:36:14]

(addressing the question first to make sure we're on the same page)

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal.h
File base/callback_internal.h (left):

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal...
base/callback_internal.h:69: scoped_refptr<BindStateBase> bind_state_;
On 2015/05/14 04:28:13, Nico wrote:
> Shouldn't this still work (with another fried for this)? Templates are
> structurally typed after all

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 04:38:23]

On 2015/05/14 04:36:14, tapted wrote:
> (addressing the question first to make sure we're on the same page)
> 
>
https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal.h
> File base/callback_internal.h (left):
> 
>
https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal...
> base/callback_internal.h:69: scoped_refptr<BindStateBase> bind_state_;
> On 2015/05/14 04:28:13, Nico wrote:
> > Shouldn't this still work (with another fried for this)? Templates are
> > structurally typed after all
> 
> 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 :))

oh wait - looking again, I think your question is about something else entirely!
Yeah! that might work - let me try it out and see if there are any sizes
regressions locally.

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

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal.h
File base/callback_internal.h (left):

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal...
base/callback_internal.h:69: scoped_refptr<BindStateBase> bind_state_;
On 2015/05/14 04:28:13, Nico wrote:
> Shouldn't this still work (with another fried for this)? Templates are
> structurally typed after all

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.

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal...
base/callback_internal.h:51: // Pointer to a function that will properly destroy
|this|. Use a regular
On 2015/05/14 04:28:13, Nico wrote:
> 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.

Done.

[tapted, 2015-05-14 05:19:59]

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal.h
File base/callback_internal.h (left):

https://codereview.chromium.org/1129353003/diff/360001/base/callback_internal...
base/callback_internal.h:69: scoped_refptr<BindStateBase> bind_state_;
On 2015/05/14 05:06:54, tapted wrote:
> On 2015/05/14 04:28:13, Nico wrote:
> > Shouldn't this still work (with another fried for this)? Templates are
> > structurally typed after all
> 
> 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.

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..

[Nico, 2015-05-14 05:25:10]

Looks great :-)

[tapted, 2015-05-14 07:17:21]

so.. I *think* my numbers were a little off since the vtables contributed a
disproportionate amount to the stuff that we strip off after extracting dSYMs in
Official builds. This might be closer to 1% :(, but still a lot.

Updated description so there's no false promises.. But let's see if the
waterfall agrees with me..

[tapted, 2015-05-14 07:17:32]

The CQ bit was checked by tapted@chromium.org

[tapted, 2015-05-14 07:17:32]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1129353003/#ps400001
(title: "Keep the DCHECK")

[commit-bot: I haz the power, 2015-05-14 07:17:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129353003/400001

[commit-bot: I haz the power, 2015-05-14 08:03:42]

Committed patchset #8 (id:400001)

[commit-bot: I haz the power, 2015-05-14 08:04:23]

Patchset 8 (id:??) landed as
https://crrev.com/e7e804c7d491ea23cfbf5b57a9ec1f8fa78b44a7
Cr-Commit-Position: refs/heads/master@{#329818}

[tapted, 2015-05-14 10:04:39]

I'll keep http://crbug.com/486594 updated with perf data. Summary so far is:

Link to perf results:
 - https://chromeperf.appspot.com/group_report?bug_id=486594

The 1MB on Mac matches with the local tests. Linux sees a 3.5MB improvement.
Android has the best %improve with ~2.5MB (	3.1% on lib_libchromeshell.so which
seems to be the thing the perf bots are interested in).

Windows doesn't seem to change much - it might not even tickle the perf bots.
Looks like a ~100kB improvement in chrome.dll

[Nico, 2015-05-14 16:43:55]

(The Linux perf bots report size before striping, there's a bug for that
somewhere. Don't know about android.)
On May 14, 2015 3:04 AM, <tapted@chromium.org> wrote:

> I'll keep http://crbug.com/486594 updated with perf data. Summary so far
> is:
>
> Link to perf results:
>  - https://chromeperf.appspot.com/group_report?bug_id=486594
>
> The 1MB on Mac matches with the local tests. Linux sees a 3.5MB
> improvement.
> Android has the best %improve with ~2.5MB (     3.1% on
> lib_libchromeshell.so which
> seems to be the thing the perf bots are interested in).
>
> Windows doesn't seem to change much - it might not even tickle the perf
> bots.
> Looks like a ~100kB improvement in chrome.dll
>
> https://codereview.chromium.org/1129353003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.