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

Refactor Dockerfile #1594

Merged
merged 6 commits into from
May 26, 2023
Merged

Refactor Dockerfile #1594

merged 6 commits into from
May 26, 2023

Conversation

ffuugoo
Copy link
Contributor

@ffuugoo ffuugoo commented Mar 20, 2023

Refactored Dockerfile:

TODO:

  • check if lld is used/works for linkage
    • and figure how to enable it if it's not used, or remove if it doesn't work
    • and/or, utilize mold linker
      • mold is not in the Debian 11 repos...
      • ...but prebuilt Linux binaries are available on GitHub
  • document Dockerfile

@ffuugoo ffuugoo requested a review from generall March 20, 2023 21:26
@ffuugoo ffuugoo force-pushed the refactor-dockerfile branch from e7a7188 to e0e0db8 Compare March 30, 2023 09:10
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Mar 30, 2023

So, I've tested the Dockerfile in most configurations imaginable: native and cross-comile builds on both ARM and x86 (on M1 Mac, through docker virtualization; takes ages :/), without lld, with lld installed and with lld enabled for Rust. However, I've mostly tested debug and no-LTO release builds, as release docker builds with "fat" LTO take literal ages (20+ minutes) on my machine.

My results:

  • lld, if present, speeds up C/C++ dependencies compilation and gives a reliably observable ~20 seconds boost (though, it takes 5-10 seconds to be installed)
  • if enabled for Rust, it seems to make absolutely no difference

I've currently pushed a change with lld enabled for Rust code, with a simple way to disable it. Could you guys test how does it work on your machines during the day?

  • docker buildx build would build in release mode, with lld enabled
  • docker buildx build --build-arg LINKER= would disable the lld
  • docker buildx build --build-arg PROFILE=dev would build in debug mode
  • (and, of course, you use both: docker buildx build --build-arg PROFILE=dev --build-arg LINKER=)

Note the times for builder 6/8 and builder 8/8 stages.

@ffuugoo
Copy link
Contributor Author

ffuugoo commented Mar 30, 2023

Oh, and BTW, is unstripped debug build for linux 900-1000 MB!?

UPD: It is. 🙈

@timvisee
Copy link
Member

timvisee commented Mar 30, 2023

I've currently pushed a change with lld enabled for Rust code, with a simple way to disable it. Could you guys test how does it work on your machines during the day?

* `docker buildx build` would build in **release** mode, with `lld` **enabled**

* `docker buildx build --build-arg LINKER=` would _disable_ the `lld`

* `docker buildx build --build-arg PROFILE=dev` would build in _debug_ mode

* (and, of course, you use both: `docker buildx build --build-arg PROFILE=dev --build-arg LINKER=`)

Note the times for builder 6/8 and builder 8/8 stages.

I ran the four commands at 09a5ed6, see my results below. Notice that I did add --no-cache:

Command Total time B6 B8
docker buildx build --no-cache . 357.3s 90.3s 240.4s
docker buildx build --no-cache --build-arg LINKER=. 351.4s 88.7s 239.9s
docker buildx build --no-cache --build-arg PROFILE=dev . 171.9s 107.4s 32.0s
docker buildx build --no-cache --build-arg LINKER= --build-arg PROFILE=dev . 219.8s 132.4s 47.4s

I ran this on Ubuntu (Linux 5.19 x86_64), AMD Ryzen 9 5900X (12/24 cores), 32GB RAM.

I guess that for release builds there are no visible improvements (outside the margin of error).

Click here for the logs.

image

image

image

image

@ffuugoo ffuugoo force-pushed the refactor-dockerfile branch from 09a5ed6 to a59442a Compare April 4, 2023 09:41
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Apr 4, 2023

Figured why release Docker builds failed on my Mac... it was RAM. Increased RAM limit available to Docker to 12GB and it works now. Didn't note the build time (before I've cleared the screen), though. 🤦‍♀️

@ffuugoo ffuugoo force-pushed the refactor-dockerfile branch 2 times, most recently from 4a2722d to 559bed6 Compare April 12, 2023 09:01
@ffuugoo ffuugoo force-pushed the refactor-dockerfile branch from 559bed6 to aca7221 Compare May 9, 2023 07:49
@ffuugoo ffuugoo mentioned this pull request May 9, 2023
@ffuugoo
Copy link
Contributor Author

ffuugoo commented May 9, 2023

Rebased to current dev. All speedups from #1856 and #1859 present.

Here's a comparison between "cold" builds. Left is this PR (aca7221, CI link), right is #1859 (d0933e5, CI link).

Screenshots Screenshot 2023-05-09 at 10 15 23

And here's a comparison between "cold" build (aca7221, CI link) and "hot" build (dbbac23, CI link) in this PR.

Screenshots Screenshot 2023-05-09 at 10 40 19 Screenshot 2023-05-09 at 10 40 24

And, for the reference, "cold" build (d0933e5, CI link) and "hot" build (d4e4453, CI link) in the #1859.

Screenshots Screenshot 2023-05-09 at 10 55 28 Screenshot 2023-05-09 at 10 55 32

@ffuugoo ffuugoo marked this pull request as ready for review May 9, 2023 15:24
@ffuugoo ffuugoo self-assigned this May 10, 2023
ffuugoo added 5 commits May 25, 2023 12:19
- fix cross-compilation
- improve caching

TODO:
- check if `lld` is used/works for linkage (and enable if not used, or remove if doesn't work)
@ffuugoo ffuugoo force-pushed the refactor-dockerfile branch from 1ee2a14 to 57c6a0e Compare May 25, 2023 10:20
@ffuugoo
Copy link
Contributor Author

ffuugoo commented May 25, 2023

Rebased on recent dev.

@generall
Copy link
Member

generall commented May 25, 2023

Here is my checklist:

  • docker buildx works for both - amd and arm
  • regular docker build . works
  • Cargo uses release profile by default, but debug profile configured in CI
  • Caches work
  • mold is used as much as possible, but at least on amd arch
  • changes in code does not invalidate dependencies cache

If those checked, then - LGTM

@ffuugoo
Copy link
Contributor Author

ffuugoo commented May 25, 2023

@generall I'll verify all of these once again (a bit) later, but unless something got accidentally broken recently, all of them should be good.

@timvisee

This comment was marked as outdated.

@ffuugoo

This comment was marked as outdated.

@ffuugoo
Copy link
Contributor Author

ffuugoo commented May 26, 2023

@generall I've tested the Dockerfile once again and marked most items in the checklist. I need help testing on AMD64 host (as I only have macOS, which is ARM).

I think sufficient tests would be:

  • docker build .
  • docker buildx build .
    • then modify any Rust file, re-run docker buildx build . and check that steps 2 to 7 of the builder stage are cached (look for the CACHED [builder 2/9] lines)
    • (it's kinda optional, but it's a quick test)
  • docker buildx build . --platform linux/arm64
  • (also, you better run all these steps with --build-arg PROFILE=ci to not wait for eternity for LTO to complete)

All of these works on my machine. E.g., "plain" docker build and native (aarch64 target) and cross (amd64 target) compilation with docker buildx.

@timvisee
Copy link
Member

@ffuugoo I'll give these a spin. I'm on Linux with amd64.

@timvisee
Copy link
Member

@ffuugoo All builds (build, buildx, buildx for arm64) work fine on my machine!

For cross compilation with buildx I did have to explicitly set a custom builder like --builder=container, which I created with docker buildx create --driver=docker-container --name=container. This has to do with my setup though, and don't think it can be improved upon in this PR.

Cache works with buildx.

I used --build-arg PROFILE=ci on all.

@timvisee timvisee self-requested a review May 26, 2023 13:02
Copy link
Member

@timvisee timvisee left a comment

Choose a reason for hiding this comment

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

Great work on this! Don't see any real problems based on a quick inspection.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Co-authored-by: Tim Visée <tim+github@visee.me>
@generall generall merged commit abffa27 into dev May 26, 2023
@ffuugoo ffuugoo deleted the refactor-dockerfile branch May 26, 2023 15:32
generall pushed a commit that referenced this pull request Jun 19, 2023
* Refactor Dockerfile

- fix cross-compilation
- improve caching

TODO:
- check if `lld` is used/works for linkage (and enable if not used, or remove if doesn't work)

* Remove `aarch64` linker config from `.cargo/config.toml` (seems to be unnecessary)

* Expose `LINKER` argument and enable `lld` linker

* Add `mold` linker support

* Document Dockerfile

* Add closing ` in the Dockerfile comments

Co-authored-by: Tim Visée <tim+github@visee.me>

---------

Co-authored-by: Tim Visée <tim+github@visee.me>
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.

3 participants