New issue
Advanced search Search tips

Issue 684788 link

Starred by 4 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Only pack used resources on non-Android too

Project Member Reported by thakis@chromium.org, Jan 24 2017

Issue description

In  bug 338759  we added a gnarly hack to only include referenced grit resources (see comment 0 over there, that hack was suggested by scottmg on some mailing list thread iirc).

1: Change grit to generate defines like:
#define IDS_SOME_RESOURCE _Pragma("message \"RESOURCE USED IDS_SOME_RESOURCE\"") 12345
instead of regular defines:
#define IDS_SOME_RESOURCE 12345

2: Extract the warnings from buildbot output.

2 used to be android buildbot specific, but https://codereview.chromium.org/2272713004/ moved into the regular build, and it should now fairly easy to do the same on Mac, Linux, Chrome OS, iOS (but I think iOS already does something else to keep their resources small -- Justin?). I think _Pragma doesn't work with cl.exe, but maybe there's something similar (or we can only do it with clang-cl, or we'll discover that it doesn't save that much size on desktop anyhow).

On Android it saves 1.5 MB.
 
I prefer "beautiful hack". It's __pragma on cl.exe.
ios doesn't do any whitelisting to my knowledge.
I gathered the unused resources on Linux the old fashioned way and built Chrome-branded / not-official .deb as is / with unused resources deleted:

The .deb size decreased from 52242172 to 52125016 -> 117156 bytes.
The unpacked size decreased from 203715089 to 203046555 -> 668534 bytes.
Status: Assigned (was: Untriaged)
Here is a prototype of resource whitelisting on Windows: https://chromium-review.googlesource.com/c/chromium/src/+/1176979

Before:
$ cat resources.pak locales/*.pak |wc -c
27553032
$ cat resources.pak locales/*.pak | gzip -c | wc -c
9590590

After:
$ cat resources.pak locales/*.pak |wc -c
26017135
$ cat resources.pak locales/*.pak | gzip -c | wc -c
9099430

So we save around 500KB compressed and 1.5MB uncompressed.

(Caveat: I haven't tried actually running the binary yet.)
I tried my change on an actual Windows machine and it looks like the browser still starts up correctly. The size of mini_installer.exe decreases from 47042560 to 46828032 -> 214528 bytes.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 17

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

commit f26ebd65019e400184adad499c73264140c8fa96
Author: Peter Collingbourne <pcc@chromium.org>
Date: Fri Aug 17 18:24:22 2018

grit: Replace current resource whitelisting implementation with a better one.

The new implementation works by embedding debug info in object
files instead of parsing compiler warning output. As a result,
our official builds are much less noisy. See the new header file
ui/base/resource/whitelist.h for details on how it works.

It is also simpler because it does not rely on wrapper scripts for
collecting the set of whitelisted resource ids. As a result we no
longer need, and can remove, the wrapper scripts for the compiler
and ar.

It is also theoretically more precise because it only whitelists
resource ids mentioned in archive members that were chosen by the
linker, although I didn't see an improvement in monochrome. It may
be possible to gain even more precision by extending the compiler
(see comments on bug) but first we'd need to investigate whether that
would be worth it.

Bug: 648475, 684788
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I12d83a8aaffa42c3afcdcc834f7dd7536b7a9248
Reviewed-on: https://chromium-review.googlesource.com/1174012
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584129}
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/android_webview/BUILD.gn
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/build/android/resource_sizes.py
[delete] https://crrev.com/a8ae8cd395ac425d4a521cbbec134c95c3e0e2f9/build/toolchain/gcc_ar_wrapper.py
[delete] https://crrev.com/a8ae8cd395ac425d4a521cbbec134c95c3e0e2f9/build/toolchain/gcc_compile_wrapper.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/build/toolchain/gcc_solink_wrapper.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/build/toolchain/gcc_toolchain.gni
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/build/toolchain/nacl/BUILD.gn
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/build/toolchain/wrapper_utils.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/chrome/BUILD.gn
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/chrome/android/BUILD.gn
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/chrome/browser/ui/webui/settings/about_handler.cc
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/chrome/browser/ui/webui/version_ui.cc
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/components/variations/service/generate_ui_string_overrider.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/ios/chrome/browser/ui/webui/version_ui.mm
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/format/gen_predetermined_ids.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/format/gen_predetermined_ids_unittest.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/format/rc_header.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/format/rc_header_unittest.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/node/misc.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/node/misc_unittest.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit/tool/build.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/grit/grit_rule.gni
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/mb/mb_config.pyl
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/resources/generate_resource_whitelist.gni
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/tools/resources/generate_resource_whitelist.py
[modify] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/ui/base/BUILD.gn
[add] https://crrev.com/f26ebd65019e400184adad499c73264140c8fa96/ui/base/resource/whitelist.h

My change from c#5 would require shipping llvm-pdbutil and llvm-undname. It looks like if we ship that in the main clang package we would increase its size by around 1.8MB. thakis, do you reckon that's small enough to include in the main package? I imagine that the alternative is that we'd require everyone who wants to build with is_official_build=true to download a separate package.

$ cat llvm-undname.stripped llvm-pdbutil.stripped | gzip -9 -c | wc -c
1833936
I started writing some guidelines on what to put in the zip (https://chromium-review.googlesource.com/c/chromium/src/+/1163172 -- I should continue working on that). According to those guidelines, this would be fine.
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 20

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

commit 09ec13ee06122eaff6e0c7d920548528fd0f61c7
Author: Peter Collingbourne <pcc@chromium.org>
Date: Mon Aug 20 21:06:43 2018

clang: Add llvm-pdbutil and llvm-undname to package.

These programs will be used to extract resource whitelists from PDBs.

Not creating a new package because llvm-undname at our pinned revision
cannot demangle resource whitelist function names without crashing.

Bug: 684788
Change-Id: I75bb11abcaca456adbea53799359a3259b942b17
Reviewed-on: https://chromium-review.googlesource.com/1180646
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584558}
[modify] https://crrev.com/09ec13ee06122eaff6e0c7d920548528fd0f61c7/tools/clang/scripts/package.py

Sign in to add a comment