Sounds cool!

A few questions:

1. What would be the impact on the binary size?

2. What happens if a piece of rodata is both in __attribute__((section("rodata_unlikely"))) and in the main .rodata? Would we be able to guarantee that it does not get folded in this case? Another option would be to put it always outside "rodata_unlikely" when there is folding.

3. naming bikeshed: should this be s/ICF/IDF/ ?

1. Haven't measured it yet. I was going to do that after the clang roll.

2. In that case they wouldn't be merged (previously we would merge them, but that was a bug that I fixed in https://reviews.llvm.org/D47241). I can't see a safe generic rule that we can implement in the linker for merging between output sections.

3. We probably don't want to rename the flags since they've been around for a while. Maybe we could redefine the C to stand for "Code+Data"? :-)

great! thank you for the answers, Peter

I reran the tryjob after the clang roll: https://chromium-review.googlesource.com/c/chromium/src/+/1069741
and there were still quite a few failures. I don't know how many root causes there are, though.

Here's what the MonochromePublic.apk size looks like if I enable --icf=safe on rodata with my upcoming safe ICF patches (patch series ending in https://reviews.llvm.org/D48146 plus some optimizations that I haven't sent for review yet).

Before: 125781815 bytes
After: 124868407 bytes

i.e. over 900KB reduction.

I also compared the size of 64-bit libmonochrome.so varying whether we do --icf=safe or --icf=all on rodata:

--icf=safe: 56616152 bytes
--icf=all: 56517480 bytes

i.e. around 100KB.

(I tried to compare the size of 32-bit libmonochrome.so in the same way but it looks like lld currently has a bug which causes it to hang if I try to link with --icf=all on rodata. Nevertheless I think we should expect to see around 100KB savings on 32-bit as well.)

So even with just --icf=safe that should get us most of the size reduction.

My safe ICF patches (including the patch that implements safe ICF on rodata with --icf=all) have landed as of clang r337640, so we should expect to see the ~900KB reduction when the next clang roll happens.

The clang roll landed in r579922, but the size reduction was only 8KB.

https://chromeperf.appspot.com/report?sid=ed9062e858640e2505914167e530b717bbb0f6d588463571ef837c7aadb2928e&rev=579922

One thing that changed since I last took measurements was that we turned on ThinLTO in official Android builds. I think I know what the problem is: we need to enable the object file metadata that allows for safe ICF in the LTO component of the linker. It should be an easy fix, so assuming that I'm correct we should expect to see the size reduction in the *next* clang roll.

https://reviews.llvm.org/D50221

Local apk size measurements are:
123656720 bytes without the fix
123063360 bytes with the fix.

Landed in r339050, so this should finally get resolved once the roll happens.

I think this is fixed now?

In terms of arm32, the clang roll containing this decreased libmonochrome.so by 265442 bytes.

https://chromeperf.appspot.com/report?sid=c818fc25c83582d53c59097a06a6fc33da0cbe0a039bd2845a32f96e3bd3044d&rev=582951