New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 14
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocking:
issue 767901



Sign in to add a comment
link

Issue 846855: Try to upstream CHROMIUM_CXX_TWEAK_INLINES to libc++

Reported by agrieve@chromium.org, May 25 2018 Project Member

Issue description

In  bug 738155 , we modified the ndk's libc++ to add __attribute__((noinline)) in several places, saving almost 1MB of binary size with little runtime performance consequence.

We should try to upstream this to llvm (presumably with a different macro name). Maybe ASSUME_LARGE_CODEBASE=1 or something (the savings come from more identical-code-folding, so only apply to larger codebases).

Perhaps as a first step, we should apply the patch against our own copy of libc++ (within buildtools/third_party/libc++) to see if the savings exist on other platforms as well.
 

Comment 1 by agrieve@chromium.org, May 25 2018

Blocking: 767901

Comment 2 by thakis@chromium.org, May 25 2018

buildtools/third_party/libc++ is an upstream mirror and can't have local diffs (by design).

Comment 3 by thomasanderson@chromium.org, May 25 2018

Owner: thomasanderson@chromium.org
Status: Started (was: Available)
It would be nice (or perhaps not so nice!) if DEPS supported patches.

I'll investigate if there's any binary size savings on Linux, and also upload an llvm patch.

Comment 4 by thomasanderson@chromium.org, May 25 2018

Cc: p...@chromium.org
I've rebased and uploaded a WIP patch here:
https://reviews.llvm.org/D47399

The binary size reduction on Linux is surprisingly good.  On an official build, this reduced my binary size from 147494776 to 142763928 bytes.  That's 4.51MiB or 3.31%.

Comment 5 by ericwf@google.com, Oct 26

If ToT Clang is the only compiler we care about, then https://reviews.llvm.org/D52405 will address the size issues when it lands on Monday.

The chrome binary size goes from 810 MB to 756 MB after this patch.

Comment 6 by thakis@chromium.org, Oct 26

ToT clang is the only compiler we care about for chrome/android (and every other chrome except chrome/ios).

810MB to 756MB sounds like a debug build though :-) Put:

  is_debug = false

in your args.gn to get a release build, and additionally

  is_official_build = true

to get the optimization level we use for shipping (this enables lto and makes links pretty slow, which is why it's a separate toggle).

Comment 7 by thakis@chromium.org, Oct 26

Also, that's fantastic!

Comment 8 by thomasanderson@chromium.org, Oct 26

:O  Awesome!!  I'll roll those changes into Chrome on Monday!  This should also unblock us on using in-tree libc++ for Android.

Comment 10 by thomasanderson@chromium.org, Oct 29

I applied the patch and I'm not seeing a significant difference.

args.gn:
use_goma = true
is_debug = false
is_component_build = false
enable_nacl = false

Before: 217588784 (208M)
After:  217208008 (208M)

Comment 11 by thomasanderson@chromium.org, Oct 31

Cc: ericwf@google.com
ericwf@ I'm not seeing a difference with official builds or debug builds either (patch https://chromium-review.googlesource.com/c/chromium/src/+/1305209).  I'm using clang version 8.0.0 (trunk 344066).  Is there anything else we have to do to get the size reduction?

Comment 12 Deleted

Comment 13 by ericwf@google.com, Oct 31

Your Clang version seems new enough. Can you clean the build artifacts and try again?

The build system doesn't seem to track changes to libc++ properly, at least to header only changes.
I did a full rebuild but that didn't seem to help (217782400 vs 217410352).  The build system should rebuild everything on libc++ rolls as long as this gets updated: https://cs.chromium.org/chromium/src/buildtools/deps_revisions.gni?rcl=13a00f110ef910a25763346d6538b60f12845656&l=9

Comment 15 by ericwf@google.com, Nov 2

OK, Weird. I double checked and I saw the changes, so I'm not sure what's really going on. What I did to double check was I disabled this `#if` block by adding `&& 0`:
https://github.com/llvm-mirror/libcxx/blob/master/include/__config#L757

Can you give me any more information about how your building, so I can help debug this?

I wasn't updating that file, Thanks for the heads up.

Comment 16 by thakis@google.com, Nov 16

thomasanderson: Looks like we didn't roll forward buildtools. Maybe you didn't have that change because of that?

Comment 17 by thomasanderson@chromium.org, Nov 19

I applied the roll locally when testing.  Discussed with ericwf@ offline and he's able to repro now in a release build.

Comment 18 by bugdroid1@chromium.org, Nov 20

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/buildtools/+/da9b2941cbf6d6d67b694e069e8e8dc06910f66a

commit da9b2941cbf6d6d67b694e069e8e8dc06910f66a
Author: Nico Weber <thakis@chromium.org>
Date: Tue Nov 20 18:58:11 2018

Revert "Roll libcxx{abi}

This reverts:

> commit fdce2ad26b26961629cb6bb2fc4757396d37f174
> Author: Tom Anderson <thomasanderson@chromium.org>
> Date:   Mon Oct 29 10:40:50 2018 -0700
>
>     Roll libcxx{abi}
>
>     This is to pick up "[libc++] Use exclude_from_explicit_instantiation instead of
>     always_inline":
>     https://chromium.googlesource.com/external/llvm.org/libcxx/+/640fa255b9c7441eeace90f96e237ca68bd8c7b6
>
>     Full changelogs:
>     https://chromium.googlesource.com/chromium/llvm-project/libcxx/+log/2199647acb904b91eea0a5e045f5b227c87d6e85..640fa255b9c7441eeace90f96e237ca68bd8c7b6
>     https://chromium.googlesource.com/chromium/llvm-project/libcxxabi/+log/c3f4753f7139c73063304235781e4f7788a94c06..794d4c0c33d99585ae99a069e99ff2c06aa98b56
>
>     BUG= 846855 
>     R=thakis

Since the CQ doesn't like it and keeping it in buildtools blocks buildtoosl
rolls.

Change-Id: I284664051b077ecdcde1320e441a7bded1652319
[modify] https://crrev.com/da9b2941cbf6d6d67b694e069e8e8dc06910f66a/deps_revisions.gni
[modify] https://crrev.com/da9b2941cbf6d6d67b694e069e8e8dc06910f66a/DEPS

Comment 20 by bugdroid1@chromium.org, Dec 13

Project Member

Comment 21 by bugdroid1@chromium.org, Dec 13

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9a085ba23156aba9ade0a08efd3d948f9527f01e

commit 9a085ba23156aba9ade0a08efd3d948f9527f01e
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Thu Dec 13 22:56:15 2018

Roll buildtools

BUG= 846855 
R=thakis

Change-Id: I6c1a1325b089b12e86268b14edbffe4b05a5e58f
Reviewed-on: https://chromium-review.googlesource.com/c/1374973
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616465}
[modify] https://crrev.com/9a085ba23156aba9ade0a08efd3d948f9527f01e/DEPS

Comment 22 by thomasanderson@chromium.org, Dec 14

Good news, in my test CL of enabling libc++ on Android, there's actually a 60KB APK size reduction https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-binary-size/118853

So I believe we're good to go on enabling in-tree libc++ for Android!

Comment 23 by agrieve@google.com, Dec 14

THAT'S AMAZING NEWS!!!! ūüéČ

Thanks for working on this!

Comment 24 by bugdroid1@chromium.org, Dec 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/buildtools/+/0e1cbc4eab6861b0c84bf2ed9a3c4b7aa2063819

commit 0e1cbc4eab6861b0c84bf2ed9a3c4b7aa2063819
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Fri Dec 14 00:43:35 2018

Stop removing hide_all_but_jni_onload config from libc++

This is no longer needed and causes gn analyze to fail on Android component
builds since the hide_all_but_jni_onload config is never added.

BUG= 846855 
R=thakis

Change-Id: I0a9961f6fbda74cd5d3c71a3420588d02bfd86f8
[modify] https://crrev.com/0e1cbc4eab6861b0c84bf2ed9a3c4b7aa2063819/third_party/libc++/BUILD.gn

Comment 25 by bugdroid1@chromium.org, Dec 14

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

commit bdfa3bb74e76d0da628f876469af1d94bd5bad83
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Fri Dec 14 19:59:26 2018

Fixes necessary to enable in-tree libc++ on Android

All that should be necessary after this change is to flip use_custom_libcxx to
true on Android.

BUG= 846855 
R=thakis

Change-Id: I386176970ea7f05a9dbd7f04a2254c3a14d5419b
Reviewed-on: https://chromium-review.googlesource.com/c/1377501
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616799}
[modify] https://crrev.com/bdfa3bb74e76d0da628f876469af1d94bd5bad83/DEPS
[modify] https://crrev.com/bdfa3bb74e76d0da628f876469af1d94bd5bad83/build/config/android/BUILD.gn

Comment 26 by thomasanderson@chromium.org, Dec 14

Status: Fixed (was: Started)

Comment 27 by bugdroid1@chromium.org, Dec 15

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

commit eebbc8a9b5d3574cb05bafb07319336f011f55f4
Author: Max Moroz <mmoroz@chromium.org>
Date: Sat Dec 15 00:09:19 2018

Revert "Fixes necessary to enable in-tree libc++ on Android"

This reverts commit bdfa3bb74e76d0da628f876469af1d94bd5bad83.

Reason for revert: https://bugs.chromium.org/p/chromium/issues/detail?id=910864#c14

Original change's description:
> Fixes necessary to enable in-tree libc++ on Android
> 
> All that should be necessary after this change is to flip use_custom_libcxx to
> true on Android.
> 
> BUG= 846855 
> R=‚Äčthakis
> 
> Change-Id: I386176970ea7f05a9dbd7f04a2254c3a14d5419b
> Reviewed-on: https://chromium-review.googlesource.com/c/1377501
> Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#616799}

TBR=thakis@chromium.org,thomasanderson@chromium.org

Change-Id: I82ad36ccb2aadc760045644e208091dc4914f154
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  846855 
Reviewed-on: https://chromium-review.googlesource.com/c/1378881
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Commit-Queue: Max Moroz <mmoroz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616886}
[modify] https://crrev.com/eebbc8a9b5d3574cb05bafb07319336f011f55f4/DEPS
[modify] https://crrev.com/eebbc8a9b5d3574cb05bafb07319336f011f55f4/build/config/android/BUILD.gn

Comment 28 by bugdroid1@chromium.org, Dec 15

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0e2335f44024cc59d7c4a8a3fd76212628f1f64c

commit 0e2335f44024cc59d7c4a8a3fd76212628f1f64c
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Sat Dec 15 00:33:24 2018

Disable libc++ on android and rollback buildtools to 7d88270d

This CL

1. Reverts 9a085ba23156aba9ade0a08efd3d948f9527f01e
2. Rolls buildtools back to 7d88270de197ebe8b439ab5eb57a4a2a0bb810e0
3. Disables libc++ on Android

BUG=910864, 846855 , 767901 
TBR=thakis
CC=‚Äč‚Äčmmoroz
NOTRY=true

Change-Id: I44e3a504fa6b842099fef78371c7dfcf7921f2f5
Reviewed-on: https://chromium-review.googlesource.com/c/1379245
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616895}
[modify] https://crrev.com/0e2335f44024cc59d7c4a8a3fd76212628f1f64c/DEPS
[modify] https://crrev.com/0e2335f44024cc59d7c4a8a3fd76212628f1f64c/build/config/android/BUILD.gn
[modify] https://crrev.com/0e2335f44024cc59d7c4a8a3fd76212628f1f64c/build/config/c++/c++.gni

Sign in to add a comment