This would save a minimum of 80kb on the current android monochrome binary (the JNI registration info that's never used in monochrome), and will probably save some amount for all binaries since I'm sure there are other unreferenced string literals.

You don't happen to have some repro smaller than "build chrome"?

How would we verify that we fixed this?

I.e. "concrete repro steps, please" :-)

80kB savings sounds pretty good though!

I don't know what gold does, but lld's implementation of --gc-sections should normally remove unused string literals, as long as they are in a section marked with the SHF_MERGE flag. clang does this unconditionally and gcc does it at -O1 or higher (or, as you mention, if you pass -fmerge-constants).

Strings in sections with SHF_MERGE and SHF_STRINGS bits are merged by the linker. I think the thing you observed is that feature.

Oh sorry, I made a pretty small repro and then forgot to attach it :)

I compiled this with:
g++ -O2 -ffunction-sections -fdata-sections -Wl,--gc-sections
(and the same for clang++)
and then dumped the .rodata section in the output. gcc ends up with .rodata just containing "hi there\0" but clang retains all the strings from kMethodsFoo as well.

typedef struct {
  const char* name;
  const char* signature;
  void* fnPtr;
} JNINativeMethod;

static const JNINativeMethod kMethodsFoo[] = {
  { "nativeInit",
    "()J",
    0 },
  { "nativeLoadsOfStuff",
    "(Lorg/chromium/foo/Bar;II)V",
    0 },
};

extern bool RegisterNatives(const JNINativeMethod*);

bool RegisterNative_Foo() {
  return RegisterNatives(kMethodsFoo);
}

int main(int argc, const char** argv) {
  argv[0] = "hi there";
  return 0;
}

Is lld the default in the clang toolchain in chromium? I didn't explicitly specify the linker.

No, we currently still use gold almost everywhere.

But we currently link official builds on Linux with lld.

I can confirm that I can reproduce this issue with bfd and gold, but not with lld. So one way to fix this might be to switch to lld on Android (for official builds, at least).

$ cat foo.sh
#!/bin/bash

for c in g++ clang++; do
  for  l in bfd gold lld; do
    $c -O2 -ffunction-sections -fdata-sections torne.cc -c
    ~/src2/llvm-project7/ra/bin/clang++ -fuse-ld=$l -Wl,--gc-sections torne.o
    
    echo "$c $l"
    strings -a a.out | grep nativeL
  done
done

$ sh foo.sh 
g++ bfd
nativeLoadsOfStuff
g++ gold
nativeLoadsOfStuff
g++ lld
clang++ bfd
nativeLoadsOfStuff
clang++ gold
nativeLoadsOfStuff
clang++ lld

(lld on ARM is still not quite there yet though IIUC.)

Filed  issue 742655  to track switching to lld on Android.

Aha; thanks for the explanation, I get how SHF_MERGE and SHF_STRINGS is able to achieve the deduplication, and it also makes sense that lld can remove unreferenced strings this way. So now I'm just disappointed that bfd/gold can't do the same thing and require gcc to emit everything into sections instead ;)

It does sound like solving the blockers and switching to lld is the right way to deal with this.

Did some related exploration in  bug 746761 . Looks like if we change jni generator to declare strings as:

const char[] kStr1 = "...";

Then the strings will get proper symbol names, and also gc'ed when linked. The (killer) caveat is that this also causes them to not be merged with identical strings.

Nothing actionable here, but I found it interesting that there was a difference between [] and const *, so thought I'd share :).

You can also add `const char* const kStr = ...`, then that should happen too. Without the second const, the pointer itself could be changed.

I didn't know that this had an influence on icf.

Have a look at  bug 746761 , having that second const doesn't help (although maybe should?).

I'm guessing maybe it's just the difference between "this is a string literal" and, "this is an array of characters". Perhaps C enforces that arrays have unique addresses?

We have 15000 const char[] as of now:
https://cs.chromium.org/search/?q=const%5C+char%5C+%5B%5E*%5D*%5C%5B%5C%5D%5C+%3D%5C+%5C%22&sq=package:chromium&type=cs

This shows that "hostname" occurs twice (admittedly not conclusive)
strings libmonochrome.so| grep hostname 

flags show up 4 times:
strings libmonochrome.so| grep '^flags$'

We've now switched to lld. The original JNI motivation has already been solved separately. Closing as no work to be done here.

Did we actually get a binsize win from switching to lld?

Big time (almost 2mb!):
https://bugs.chromium.org/p/chromium/issues/detail?id=742655#c23

Yes, the monochrome APK size decreased by 1.6MB.

https://chromeperf.appspot.com/group_report?rev=531064

I wonder if you guys have enabled string tail merging. Please make sure that you passed -O2 to lld to enable the feature.

We're currently passing -O1. Looks like -O2 is a 150KB win in libmonochrome.so, let's go for it.

Filed issue 810154.

When I build Android Chrome on my machine, it looks like generate .so files contain not .gnu.hash but .hash sections. Is this intentional? Using .gnu.hash may shrink the file size even more.

Which option controls which style of hash is emitted?

--hash-style={sysv,gnu,both}. Default is both so that a generated binary file runs on old operating systems. GNU hash is a new and better one, so unless we are creating DSOs for very old system, we should use --hash-style=gnu.

Why do operating system launchers care about the executable hash?

Resolving a dynamic symbol is expensive without these sections. We have a list of DSOs from which dynamic symbols should be resolved, and for each dynamic symbol, we look up the DSOs's symbol table one by one until we find a match. So it is O(n*m) where n and m are the number of DSOs and the number of dynamic symbols. For large programs, both n and m are large.

To reduce the cost of lookup, DSOs have on-disk hash tables (and bloom filters if .gnu.hash). That saves a lot of time because hash table lookup is fast.

Oh, it's a hash table, not a hash. I had confused this with the -Wl,--build-id hash. Thanks for clarifying!

So we should use --hash-style=gnu.

Maybe that should be the default if it works on all but "very old" OSs? If someone needs to build for such an old OS, they're likely using an older linker, and if they aren't they could use the flag to ask for the old behavior. Maybe the flag default should be good for the common case.

https://chromium-review.googlesource.com/#/c/chromium/src/+/907449 , but changing the default since most programs on this world won't pass this flag explicitly.

Users don't need to worry about it in most cases because (I believe) the compiler automatically passes --hash-style=both to the linker. Unless you want to save space by eliminating .hash, that default flag should work fine. So, maybe we should change the linker's default to --hash-style=gnu to modernize it, it is perhaps not that important.

Yes, I meant we should change the compiler driver, so that =gnu becomes the default for saving space if it's no longer needed in practice almost all of the time.

But now that I look at the driver source:
http://llvm-cs.pcc.me.uk/tools/clang/lib/Driver/ToolChains/Linux.cpp#237

  // Android loader does not support .gnu.hash.

O_o

https://android-review.googlesource.com/c/platform/build/soong/+/456718/2/cc/linker.go says it works on "newer devices"; I'll ask around to find out what that means.

Sigh. lld's sysv-style .hash section is very unoptimized because we focus on producing good .gnu.hash option and maintain .hash section just for backward compatibility. We should improve lld so that it produces better .hash section.

Android supports .gnu.hash since M. So we should use --hash-style=both for Android for the time being. Sounds like we currently only emit the sysv hash on Android, so =both might help startup a bit.

The monochrome APK requires N, so we could potentially use gnu in libmonochrome and both in libchrome.

Enabling for libmonochrome is quite convenient, as libchrome hides all JNI symbols, leaving *very* few exported symbols. libmonochrome.so on the other hand exports all JNI symbols, so has significantly more.

I have a build around..

This many symbols is exported:

shell> objdump=third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-objdump

shell> $objdump -j .text -T gn_android/ReleaseOfficial/libmonochrome.so | grep '^[0-9a-f]' | wc -l
1917

shell> $objdump -j .text -T gn_android/ReleaseOfficial/libchrome.so | grep '^[0-9a-f]' | wc -l
1

oh, that's *very* few :)

libmonochrome hash sizes:

shell> $objdump -h gn_android/ReleaseOfficial/libmonochrome.so | grep hash | awk '{ sz = strtonum("0x"$3); printf ("%s - %s bytes\n", $2, sz); }'
.gnu.hash - 11648 bytes
.hash - 18280 bytes

Saving 11K with --hash-style=gnu seems worth it :)

Also found that all the symbols are auto-generated JNI names, 160KiB in total, urges my inner geek to compress them, but then the inconvenience of maintaining our own demangler for 150KiB of binary size .. probably is not worth it

pardon, it would be saving 18KiB

Yeah, it should be fine to switch hash style for monochrome.

Trying to compress those symbols would defeat the point of them being there - that the VM knows how to look them up on demand, avoiding a startup time penalty to register them all manually. Also, the manual registration code used in other builds I believe is of a comparable size - it has to contain all the Java type information, class names, and function names in the JNI symbol names anyway as string literals to pass to the VM, so while there's a bit less string data due to not repeating the class names as much and the strings being slightly more likely to be duplicates that can be collapsed, the added code to actually do the registration probably makes up for it..

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

commit f0423b008b159db5e4967e87ff503140679d22d6
Author: Andrew Grieve <agrieve@chromium.org>
Date: Fri Feb 09 05:19:58 2018

Set --hash-style=gnu for libmonochrome.so

Shrinks libmonochrome.so by 16kb.

Bug:  742525 
Change-Id: I8c772feac1b09c2ebe68bb237229839050e541db
Reviewed-on: https://chromium-review.googlesource.com/909711
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535648}
[modify] https://crrev.com/f0423b008b159db5e4967e87ff503140679d22d6/chrome/android/BUILD.gn

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

commit 02d3c5b9207ffa9d4eb3ce7480bf2c21fec2902b
Author: Peter Collingbourne <pcc@google.com>
Date: Tue Mar 20 14:46:18 2018

Also pass --hash-style=gnu when building the android_webview libmonochrome.so.

Shrinks the apk size by another 12kb.

Bug:  742525 
Change-Id: I97e889cca1adb52da69490c57094565121d0b52a
Reviewed-on: https://chromium-review.googlesource.com/970083
Commit-Queue: Richard Coles <torne@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544361}
[modify] https://crrev.com/02d3c5b9207ffa9d4eb3ce7480bf2c21fec2902b/android_webview/BUILD.gn