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

Docker: statically link the C runtime #523

Closed
wants to merge 3 commits into from

Conversation

julianbraha
Copy link
Contributor

During testing, I measured a 1.5% speedup to zero-bin by statically linking the C runtime.

@0xaatif
Copy link
Contributor

0xaatif commented Aug 21, 2024

Thanks Julian! Could you share details of your test methodology?

@julianbraha
Copy link
Contributor Author

No

Dockerfile Outdated
# use the cache mount
# (we will not be able to to write to e.g `/src/target` because it is bind-mounted)
CARGO_TARGET_DIR=/artifacts \
cargo build --locked "--profile=${PROFILE}" --target=x86_64-unknown-linux-gnu --all
Copy link
Contributor

@0xaatif 0xaatif Aug 21, 2024

Choose a reason for hiding this comment

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

I don't think we should fix the target like this - we have users on amd64 that this build needs to work for

Is this required for crt-static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good point, but yeah the target needs to be set explicitly for crt-static.

Maybe the target should be made an argument to the Dockerfile, instead?

@Nashtare Nashtare added this to the Performance Tuning milestone Aug 21, 2024
@julianbraha julianbraha requested a review from 0xaatif August 23, 2024 15:09
Dockerfile Show resolved Hide resolved
Copy link
Contributor

@0xaatif 0xaatif left a comment

Choose a reason for hiding this comment

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

I'm a bit skeptical of the changes as written:

  • TARGET defaults to x86_64, so we break amd64 arm64 users immediately, and they have to override (every time!) it to get it working. This is not the UX I want for devs
  • We ignore the rustflags in .config/cargo.toml, which leaves us open to this regression: fix: linkme lld issue in CI zero-bin#79 (I'm not sure if that specific issue still exists, but I want only one place for RUSTFLAGS in general)
  • I don't have more details of the speedup

I like the Dockerfile as a quick way, general way for users to get started, and I think this performance improvement belongs elsewhere.

I think we have space for an optimised build job, which e.g

  • only targets x86_64
  • single-threads the compiler
  • statically links the crt
  • any linker shenanigans that help us

But I don't think that space is here - perhaps a bash script or even a doc for very performance-sensitive users - how do you feel about moving your suggestion to some documentation, for example?

@atanmarko
Copy link
Member

@julianbraha I would agree with @0xaatif, that this Docker image's primary purpose is to be a quick starter for novel users of the project that should work out of the box. 1.5% speedup is not worth breaking up wider compatibility and usability in this particular case. But certainly, this optimization is good to know, and we should use it somewhere else.

@julianbraha
Copy link
Contributor Author

TARGET defaults to x86_64, so we break amd64 users immediately, and they have to override (every time!) it to get it working. This is not the UX I want for devs

I think there's a misunderstanding here - the amd64 target for Rust is called x86_64.

this Docker image's primary purpose is to be a quick starter for novel users of the project that should work out of the box

I was thinking that anyone running this in a production environment would be using Docker/Kubernetes. But perhaps it would be better to split it into a "quickstart" Dockerfile and a high-optimization Dockerfile.

Feel free to close this.

@0xaatif
Copy link
Contributor

0xaatif commented Aug 26, 2024

I think there's a misunderstanding here - the amd64 target for Rust is called x86_64.

You're correct, I misspoke - I meant to write arm64 - I've updated my original message

I'll create an issue to track builds/docs for optimised users

@0xaatif
Copy link
Contributor

0xaatif commented Aug 26, 2024

#544

@0xaatif 0xaatif closed this Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants