Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cargo: enable LTO for release builds #105

Merged
merged 1 commit into from Jul 31, 2018
Merged

Conversation

csssuf
Copy link
Contributor

@csssuf csssuf commented Jul 20, 2018

LTO makes the final binary a bit smaller, and isn't really a risk here,
so enable it.

@csssuf csssuf requested a review from sdemos July 20, 2018 17:23
@cgwalters
Copy link
Member

cgwalters commented Jul 23, 2018

Relatedly, just my 2¢ - in rpm-ostree we do:

[profile.release]
panic = "abort"
lto = true

As I think for binaries (and particularly daemons managed by systemd) you really want a core dump collected. panic=abort definitely makes things faster as lots of conditional branches get elided. Edit: more information.

Copy link
Contributor

@sdemos sdemos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the size difference with the binary now with panic=abort?

@csssuf
Copy link
Contributor Author

csssuf commented Jul 23, 2018

> $ ls -lh target/release/coreos-metadata*
-rwxr-xr-x 2 james james 3.4M Jul 23 14:30 target/release/coreos-metadata
-rwxr-xr-x 2 james james 3.0M Jul 23 14:02 target/release/coreos-metadata-abort

@lucab
Copy link
Contributor

lucab commented Jul 24, 2018

I'm wary of switching the panic behavior to abort. It will get in the way of any library we consume trying to catch its own panic, and it will also prevent any destructor to properly run (think of lockfile and other steps to undo).

The rpm-ostree is a different case and that setting makes sense there, as that one is a staticlib consumed by C and unwinding through FFI is not supported.

@cgwalters
Copy link
Member

think of lockfile

Lock files are cleaned up by the kernel.

@sdemos
Copy link
Contributor

sdemos commented Jul 25, 2018

update-ssh-keys also relies on the destructor being run to switch back to the original user after calling setuid and setgid to write the ssh keys. that seems like something that might also get cleaned up by the os though? I'm not sure.

@csssuf
Copy link
Contributor Author

csssuf commented Jul 25, 2018

Since it seems like we're not sure if we want panic = abort, I'll just revert back to only enabling LTO in this PR and we can turn on panic = abort in a followup if we decide we do want it.

@cgwalters
Copy link
Member

update-ssh-keys also relies on the destructor being run to switch back to the original user after calling setuid and setgid to write the ssh keys. that seems like something that might also get cleaned up by the os though? I'm not sure.

Yeah, calling setuid only affects the current process; if the entire process goes away there's nothing to clean up.

@cgwalters
Copy link
Member

It's probably worth mentioning a case that isn't handled with simple process death: rust-lang/rust#49032 (comment)

(Though IMO here the fix is a kernel API to say that the tty mode changes made by a process should be transient; a userspace workaround would be to fork off a helper process whose sole job is to monitor the parent process and reset the tty mode on exit)

LTO makes the final binary a bit smaller, and isn't really a risk here,
so enable it.
@csssuf
Copy link
Contributor Author

csssuf commented Jul 31, 2018

I've opened #107 to track panic=abort; since LTO seems agreed-upon I'll merge this as-is.

@csssuf csssuf merged commit bf4e70c into coreos:master Jul 31, 2018
@csssuf csssuf deleted the release-lto branch July 31, 2018 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants