Issue metadata
Sign in to add a comment
|
32875 byte regression in resource_sizes (MonochromePublic.apk) at 579061:579061 |
||||||||||||||||||||
Issue descriptionThis 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?
,
Jul 30
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}
,
Jul 31
+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
,
Jul 31
@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).
,
Jul 31
@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.
,
Jul 31
@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
,
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
,
Aug 2
Thanks for looking into this and reclaiming those bytes!
,
Aug 5
,
Aug 30
Issue 879176 has been merged into this issue. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Jul 30