1268547 - rust: build staticlib with -Clto

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ralph Giles (:rillian)]

Turns out we can work around rust's current two-level symbol visibility, which causes std library symbols to be public in .so output, but running the aggregrate crate through lto when creating the static library we link into libxul.

Comment 1

[Ralph Giles (:rillian)]

Attachment: MozReview Request: Bug 1268547 - Enable lto linking our final rust crate. r?froydnj

Passing -Clto tells the rust compiler to run the entire
set of crates through the llvm optimizer. This has the
practical effect of removing a bunch of otherwise exported
std library symbols.

It resulst in a modest increase in binary size in unoptimized
builds but a significant decrease in the size of optimized builds.

Review commit: https://reviewboard.mozilla.org/r/49479/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49479/

Comment 2

[Ralph Giles (:rillian)]

Thanks to Alex Crichton for the pointer.

Nathan, do we want this for all builds, or just optimized builds?

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8746585 [details]
MozReview Request: Bug 1268547 - Enable lto linking our final rust crate. r?froydnj

https://reviewboard.mozilla.org/r/49479/#review46351

I wonder why the size goes up in non-optimized builds?  That's kind of weird.

Please fix the typos in the commit message before landing.

Comment 5

[Ralph Giles (:rillian)]

Sadly, this gives multiple symbol warnings linking the gtests libxul.so. Not sure why we didn't trigger this before.

 15:13:09     INFO -  librul.a(rul.0.o): In function `unix::notbsd::WEXITSTATUS::h661938ca194ff7cb0rb':
 15:13:09     INFO -  rul.0.rs:(.text._ZN4unix6notbsd11WEXITSTATUS20h661938ca194ff7cb0rbE+0x0): multiple definition of `unix::notbsd::WEXITSTATUS::h661938ca194ff7cb0rb'
15:13:09 INFO - ../librul.a(rul.0.o):rul.0.rs:(.text._ZN4unix6notbsd11WEXITSTATUS20h661938ca194ff7cb0rbE+0x0): first defined here
[...and many more...]

Alex, any idea what's going on?

Comment 6

[Alex Crichton [:acrichto]]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> I wonder why the size goes up in non-optimized builds?  That's kind of weird.

This *may* be because of how we re-codegen the entire standard library. Nothing is stripped as no optimizations are performed, and I guess LLVM's codegen is worse at lower opt-levels than it is at higher opt-levels?

This is surprising as well to me, though, albeit a rarely encountered use case.

(In reply to Ralph Giles (:rillian) needinfo me from comment #5)
> Alex, any idea what's going on?

Is Gecko linking multiple crates compiled with `-C lto`? Or perhaps linking rlibs plus a crate compiled with `-C lto`?

The object file in the staticlib produced will contain all upstream dependencies, so if two crates are linked you'll get duplicate symbols.

Comment 7

[Ralph Giles (:rillian)]

(In reply to Alex Crichton [:acrichto] from comment #6)

> Is Gecko linking multiple crates compiled with `-C lto`? Or perhaps linking
> rlibs plus a crate compiled with `-C lto`?

Looks like this is the cause. We compile one 'librul.a' from the all the rlibs for xul, and another 'librul.a' for the gtests, and link them both into the gtest version of libxul.so. If I manually put both crates a single unified staticlib, the builds succeeds. So we need to clean up our crate generation.

Comment 8

[Nathan Froyd [:froydnj]]

This will be a single-line change once we get cargo support in bug 1231764.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #8)
> This will be a single-line change once we get cargo support in bug 1231764.

e.g. https://treeherder.mozilla.org/#/jobs?repo=try&revision=25a7ec2bf234

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: turn on LTO support for the Rust parts of libxul

Nothing complicated here.  This depends on building libxul via Cargo, which
just landed on inbound this morning.

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8777404 [details] [diff] [review]
turn on LTO support for the Rust parts of libxul

Review of attachment 8777404 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/library/rust/Cargo.toml
@@ +35,2 @@
>  debug-assertions = false
>  codegen-units = 1

I mentioned this somewhere else, but the cargo docs claim codegen-units is ignored if lto is true. It might be clearer to omit codegen-units when lto is set.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (:chmanchester) from comment #11)
> I mentioned this somewhere else, but the cargo docs claim codegen-units is
> ignored if lto is true. It might be clearer to omit codegen-units when lto
> is set.

Sorry about that; I remember your previous advice, but I clearly failed to apply it to this new context. :)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee73a5429f54
turn on LTO support for the Rust parts of libxul; r=chmanchester

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee73a5429f54

Comment 15

[William Lachance (:wlach)]

Nice incremental installer size win from this:

== Change summary for alert #2323 (as of August 08 2016 07:35 UTC) ==

Summary of tests that improved:

  installer size summary osx-10-7 opt: 111891020.83 -> 110739836.83 (1.03% better)
  installer size summary linux32 opt: 56733463.33 -> 56129677.08 (1.06% better)
  installer size summary linux32 pgo: 60078857.58 -> 59453749.58 (1.04% better)
  installer size summary linux64 pgo: 59887845 -> 59308565.17 (0.97% better)

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=2323

Comment 16

[Nathan Froyd [:froydnj]]

(In reply to William Lachance (:wlach) from comment #15)
> Nice incremental installer size win from this:
> 
> == Change summary for alert #2323 (as of August 08 2016 07:35 UTC) ==
> 
> Summary of tests that improved:
> 
>   installer size summary osx-10-7 opt: 111891020.83 -> 110739836.83 (1.03%
> better)
>   installer size summary linux32 opt: 56733463.33 -> 56129677.08 (1.06%
> better)
>   installer size summary linux32 pgo: 60078857.58 -> 59453749.58 (1.04%
> better)
>   installer size summary linux64 pgo: 59887845 -> 59308565.17 (0.97% better)
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=2323

Cool, thanks for the pointer!  Do we not track Windows installer size at all?

Comment 17

[William Lachance (:wlach)]

(In reply to Nathan Froyd [:froydnj] from comment #16)
> Cool, thanks for the pointer!  Do we not track Windows installer size at all?

We do but not seeing any wins there (afaict):

https://treeherder.mozilla.org/perf.html#/graphs?series=%5Bmozilla-inbound,83688d879936c1a2bf175a6652d63f69daf217b9,1,2%5D

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Do we want to uplift that?

Comment 19

[Nathan Froyd [:froydnj]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #18)
> Do we want to uplift that?

We could, but it would require uplifting bug 1231764 as well, and possibly another Mac-specific bug or two (we upgraded the version of Rust we used on Mac builds around the same time this landed, and I don't know whether the older version would a) work with -C lto; or b) produce the same size wins).  I could imagine doing it for Aurora, but doing this on Beta seems a bit risky to me.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

OK, quite some work for a small gain, let it side the train :)