New issue
Advanced search Search tips

Issue 869098 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 5
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

32875 byte regression in resource_sizes (MonochromePublic.apk) at 579061:579061

Project Member Reported by cjgrant@google.com, Jul 30

Issue description

This binary size increase was a result of adding MaybeValid() to base::Callback.

Commit: 40f8e9a571636737228190af5a062869dbf028d4

Debugging size regressions is documented at: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/apk_size_regressions.md#Debugging-Apk-Size-Increase

Nicolas, this is seemingly due to a size increase in a large number of template instantiations.  Was the increase expected?  Is there another way to add the validity check that doesn't impose the increase?
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=869098

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=c46793189d5377fb1adec4a323259843dc338c40f2c1fdce2ce80d0f28ea7122


Bot(s) for this bug's original alert(s):

Android Builder Perf
Assigning to nicolaso@chromium.org because this is the only CL in range:
Add MaybeValid() to base::Callback

This is a thread-safe validity check for WeakPtr-based Callbacks using the recently added WeakPtr::MaybeValid().

Bug:  730693 
Change-Id: I174efb30bb16d2776e33ec64d48a913943e770a0
Reviewed-on: https://chromium-review.googlesource.com/1144208
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Taiju Tsuiki <tzik@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579061}
Cc: gab@chromium.org
+gab@

This was briefly talked about during code review [1], although that discussion was about runtime resource usage rather than binary size.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1144208/6/base/callback_internal.h#92
Cc: nicolaso@chromium.org
Owner: tzik@chromium.org
@tzik : can you think of a better structure for this code?

Should BindStateBase instead take the CancellationTraits as a template param (and then we can get rid of both the is_cancelled and maybe_cancelled pointers, turning this regression into a gain by fixing it 2x :))

If you don't have cycles to work on this then I think we should revert r579061 for now as nicolaso@ was only with us for a one week Noogler rotation (through which we've already made it further than I expected -- thanks again; learning, reverting, and iterating is a normal part of the Chromium development process).
@gab: I don't mind working on a fix for this, if we all agree on how to fix it, and assuming the change isn't too involved.

We can also revert in the meantime if we want to reduce binary size, since the change doesn't add functionality by itself.
@nicolaso@, to be totally sane, could you locally create a revert CL, then run diagnose_bloat on it?  You should see a 32 KB reduction, and in doing so, have the tool needed to measure the impact of any work you do.

https://cs.chromium.org/chromium/src/tools/binary_size/README.md?sq=package:chromium&g=0
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9697d49e6ef7204178223823afd4c11269a8fd33

commit 9697d49e6ef7204178223823afd4c11269a8fd33
Author: tzik <tzik@chromium.org>
Date: Thu Aug 02 13:35:19 2018

Tweak Callback's MaybeValid implementation for smaller binary size

http://crrev.com/40f8e9a571636737 gained the stripped binary size of
chrome by 40kB (147306728 to 147347688), which is mainly due to an extra
parameter set up on BindStateBase for each base::Bind() caller.

This CL removes the extra argument by merging it to single function with
another parameter. So that the binary loses back to the original size.
(147347688 to 147306728).

Bug:  869098 
Change-Id: I7b60797c16a00472e9312466fdd2434395d8bbf7
Reviewed-on: https://chromium-review.googlesource.com/1158093
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580152}
[modify] https://crrev.com/9697d49e6ef7204178223823afd4c11269a8fd33/base/bind_internal.h
[modify] https://crrev.com/9697d49e6ef7204178223823afd4c11269a8fd33/base/callback_internal.cc
[modify] https://crrev.com/9697d49e6ef7204178223823afd4c11269a8fd33/base/callback_internal.h
[modify] https://crrev.com/9697d49e6ef7204178223823afd4c11269a8fd33/base/callback_unittest.cc

Thanks for looking into this and reclaiming those bytes!
Status: Fixed (was: Assigned)
 Issue 879176  has been merged into this issue.

Sign in to add a comment