Paths

Table of Contentst

cargo.mk: Make use of WITH_LTO by default
ClosedPublic
Actions

Authored by diizzy on Sep 26 2022, 11:19 PM.

Details

Reviewers

tobik
asomers
0mp
driesm
lcook
jbeich
eduardo
val_packett.cool
yuri
ashish
otis
yasu
pizzamig
pkubaj

Group Reviewers

rust

Commits

R11:967022fd812c: Mk/Uses/cargo.mk: Enable WITH_LTO by default

Summary

Many ports seem to benefit from this change and it would also reduce binary size significantly, add option to opt out in case it breaks port

A few examples on amd64 (binary size)

2.2M --> 1.7M work/stage/usr/local/bin/ztop - sysutils/ztop
7.3M --> 4.0M work/stage/usr/local/bin/librespot - multimedia/librespot
11M  --> 7.9M work/stage/usr/local/bin/mdbook - textproc/mdbook

I've added people who either have rust ports or have made rust related commits if I've forgot someone feel free to add

Test Plan

exp-run, at least some runtime testing

Diff Detail

Repository

R11 FreeBSD ports repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

diizzy requested review of this revision.Sep 26 2022, 11:19 PM

diizzy created this revision.

Isn't there LTO_UNSAFE to opt out of WITH_LTO?

I actually forgot about it, should we use that approach instead?

Don't enable LTO on powerpc64 and probably also riscv64.
I didn't exclude them from lto.mk because LTO is not default but here you're making it default.

In D36736#834107, @pkubaj wrote:

Don't enable LTO on powerpc64 and probably also riscv64.
I didn't exclude them from lto.mk because LTO is not default but here you're making it default.

Should we handle it here or in Mk/Features/lto.mk since it's a "global" issue?

Adapted to suggestions by tobik and pkubaj

Make use of LTO_UNSAFE, disable LTO on powerpc64 and riscv64 add option to override.

No change, just generate a patch using -U9999 for more context on recommendation by tcberner

tobik added inline comments.Sep 30 2022, 10:45 AM

Mk/Uses/cargo.mk
142	Why not just set WITH_LTO and let `lto.mk` handle the rest? _CARGO_MSG doesn't seem that useful and is maybe something `lto.mk` can print in general.

diizzy added inline comments.Sep 30 2022, 10:56 AM

Mk/Uses/cargo.mk
142	It's needed otherwise IGNORE gets set which will result in no ports being built at all. I think it might be a good idea to have some kind of visual indicator or what's going on but it if it's deemed unnecessary we can drop it. I did try to add output via lto.mk but I think we might be a bit too early in as I get "Invalid line type" trying to use echo/${ECHO_MSG}

diizzy added inline comments.Sep 30 2022, 11:00 AM

Mk/Uses/cargo.mk
142	We could exclude !defined(LTO_UNSAFE) though looking at it again as that gets evaluated in lto.mk

tobik added inline comments.Sep 30 2022, 11:24 AM

Mk/Uses/cargo.mk
142	Philosophically I guess I'm dumbfounded as to what the purpose of `lto.mk` is (or Mk/Features in general). I thought setting WITH_LTO is like saying "I want to use LTO if at all possible but am fine without it if it isn't". I thought it was a hint not a must. Now every port that sets WITH_LTO must wrap it in the same conditional to not get IGNOREd on powerpc64. `lto.mk` could maybe export a variable when LTO is really, really used by the build. Just so you don't have the same conditional everywhere.

When I added lto.mk, it was with the assumption that it will not be used by individual ports, but rather set in make.conf by the user and after some time (probably years) set globally for all the ports (just like SSP is). Here you're taking kind of the middle route - enabling globally, but only for some ports and I didn't think about that :)

diizzy added inline comments.Sep 30 2022, 11:53 AM

Mk/Uses/cargo.mk
142	That "if at all possible" part is a bit hard to find a nice way balance to. While you could silently ignore targets I do think that it introducing silent workarounds causes more harm than good especially when it comes to debugging. What it boils down to is whether we should play it safe and tell the user "this is a bad idea" and bail out or silently(?) ignore WITH_LTO. I personally would much prefer the first option rather than "why doesn't this work" and having to dig through code to figure out why. If we at least could output something like "this is disabled on XXX" I guess that would be a middle ground but I dont see how we can do that in a nice way.

tobik added inline comments.Sep 30 2022, 12:15 PM

Mk/Uses/cargo.mk
142	If we at least could output something like "this is disabled on XXX" I guess that would be a middle ground but I dont see how we can do that in a nice way. IIUYC, this can be done by appending something to WARNING or DEV_WARNING.

I've been given a ok to change IGNORE to WARNING in lto.mk but we still need something to hook onto to determne final "status" if we want to output some kind of notification or is that unnecessary?

@mikael asked for some performance data and here's a small sample from my slow build box (Intel G3220 with 12Gb of RAM) with random sample of ports:

Stock:
02:51:58

LTO enabled:
02:40:35

NOTE: texlab failed in LTO build (add about 4 minutes) so we need to implement a toggle for CARGO_PROFILE_RELEASE_PANIC (unwind instead of abort)

Ports tested:

audio/lewton
audio/mousai-devel
audio/myxer
audio/ncspot
audio/shortwave
deskutils/sigi
deskutils/solanum
devel/sentry-cli
devel/stylua
devel/texlab
editors/kak-lsp
editors/kibi
editors/lapce
graphics/ikona
graphics/libopenraw
misc/broot
misc/ruut
multimedia/scte35dump
multimedia/termplay
security/rustscan
security/sequoia
security/sniffglue
textproc/htmlq
textproc/jless
textproc/jql
textproc/mdbook
www/lychee
www/miniserve
www/rearx
www/rustypaste
x11/inputplug
x11/kickoff
x11/salut

In D36736#845254, @diizzy wrote:
@mikael asked for some performance data and here's a small sample from my slow build box (Intel G3220 with 12Gb of RAM) with random sample of ports:

Stock:
02:51:58

LTO enabled:
02:40:35

NOTE: texlab failed in LTO build (add about 4 minutes) so we need to implement a toggle for CARGO_PROFILE_RELEASE_PANIC (unwind instead of abort)

Ports tested:
audio/lewton
(...)
x11/salut

I'm about to do some testing to all cargo ports, portgrep -u cargo -os -R . | wc -l: ~219

Plan:

Build cargo.list with LTO
Remove LTO failed ports from cargo.list (so we can have a functional list with/without LTO for comparison)
Build with LTO
Build without LTO

3,4:

Compare compile time
Compare disk usage * It will be nice to have an idea on binaries diff, maybe comparing pkgs size but I can't find a good way to compare those.

pkgs are build in, e.g, /usr/local/poudriere/data/packages/131amd64-devel/All/ so I think it's possible to compare WITH and WITHOUT LTO pkgs in here,
since I will use poudriere bulk -C -f cargo-list ...

Any ideas? Maybe this will take about 6-8 hours per build.

I think we should try to aim for using LTO as much as possible so we should probably add a toggle later on. At least in texlab's case it clearly said that it wanted unwind instead of abort so by doing a full run we should get more data if it's "that simple" to determine. Other than I think it's it looks like a good proposal.

@eduardo
Did you ever get around doing some build tests?

In D36736#851428, @diizzy wrote:

@eduardo
Did you ever get around doing some build tests?

@diizzy
No, but I can start builds tomorrow.
I think I will have results in 2-3 days

Tomorrow I will have comparison results for 215 cargo ports now that I have a fixed list of pkgs to build.
https://people.freebsd.org/~eduardo/LTO/

# poudriere-git-3.3.99.20220831
# ports @ 0872571907226a3f50dc379fdea954d0f03db199
#
# cargo list:                   LTO/cargo.txt
# pkg list:                     LTO/pkg.txt
# method:                       LTO/methodology.txt
#
# [1]: Fails on default, logs:  LTO/logs/errors/default/
# [2]: Fails on lto, logs:      LTO/logs/errors/lto/
# [3]: Ignored
# [D]: Dependency to other ports not listed here, more builds
#
# databases/postgresql-promscale [1]
# devel/texlab [2]
# games/abstreet [3]
# games/veloren [2]
# graphics/libimagequant [D]
# net/findomain [1]
# security/solana [3]
# sysutils/vector [2]

Results: 215 ports

build | time | size
default | 05h:06m | 715MB
lto | 05h:56m | 591MB

(more details on URL above)

Slightly more time compiling with a ~22% smaller pakages.

For what I've read in https://blog.rust-lang.org/inside-rust/2020/06/29/lto-improvements.html
there are 3 LTO modes: Fat, Thin and Thin-local (default) so compile times and binary reduction may vary depending on what's used.

Any benchmarks on runs to test applications performance?

That's great data, thanks!
I tried thin but it performed noticably worse and wasn't much faster but it probably scales better on multicore systems.

arrowd added a subscriber: arrowd.Dec 7 2022, 8:44 AM

arrowd added inline comments.

Mk/Uses/cargo.mk
142	I'd add some parentheses into this line.

diizzy added inline comments.Dec 7 2022, 9:24 AM

Mk/Uses/cargo.mk
142	Not sure what you want me to change, example?

arrowd added inline comments.Dec 7 2022, 9:41 AM

Mk/Uses/cargo.mk
142	I find . if !defined(LTO_UNSAFE) \|\| (defined(LTO_DISABLE_CHECK) && ${ARCH} == powerpc64) \|\| (defined(LTO_DISABLE_CHECK) && ${ARCH} == riscv64) more appealing, but that's just a nitpick.

pkubaj accepted this revision.Dec 22 2022, 3:11 PM

This revision is now accepted and ready to land.Dec 22 2022, 3:11 PM

@eduardo @pkubaj
Mikael gave this a go on IRC however my current plan is to wait until 2023Q1 is branched

@diizzy

Sounds a good plan and nice that this feature will land soon.

Cheers

Update, I'm currently running a new build due to the amount of ports that's been recently added to avoid unnecessary breakage. It'll probably take a day or two before it finishes.

Closed by commit R11:967022fd812c: Mk/Uses/cargo.mk: Enable WITH_LTO by default (authored by diizzy). · Explain WhyJan 7 2023, 7:24 PM

This revision was automatically updated to reflect the committed changes.

diizzy added a commit: R11:967022fd812c: Mk/Uses/cargo.mk: Enable WITH_LTO by default.