Skip to content

Add YJIT support to debian 3.2+ #400

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

Merged
merged 2 commits into from
Jan 13, 2023
Merged

Add YJIT support to debian 3.2+ #400

merged 2 commits into from
Jan 13, 2023

Conversation

paihu
Copy link
Contributor

@paihu paihu commented Dec 28, 2022

use latest rust

related #390

@tianon
Copy link
Member

tianon commented Dec 29, 2022

Duplicate of #398

Unfortunately, this method of installing Rust is not going to meet the requirements of https://github.com/docker-library/official-images#review-guidelines (especially https://github.com/docker-library/official-images#security) -- if we do this, it will have to be quite a bit more complex, especially explicit version pinning and download verification, more similar to what's seen in https://github.com/rust-lang/docker-rust/blob/79f14a915cfe70e27d0ee40ab7d43a15b41cc1fa/1.66.0/bullseye/slim/Dockerfile. Additionally, the Ruby image supports more architectures than Rust appears to, so we'll have to get clever with the conditionals if we want to make this happen. All of this is factored into why we've opted not to enable YJIT in the Debian variants for now.

@BrianHawley
Copy link

BrianHawley commented Dec 29, 2022

@tianon that rust image code you linked isn't a lot more complex than the code in this PR. If this PR were adjusted to use that rust installation technique, or another PR created with code like that, would that be sufficient to enable YJIT in the Debian variants?

@paihu
Copy link
Contributor Author

paihu commented Dec 30, 2022

@tianon thank you infomation. fix it.

Additionally, the Ruby image supports more architectures than Rust appears to, so we'll have to get clever with the conditionals if we want to make this happen.

YJIT written by rust. so YJIT support architecture is same or less than rust.

@josacar
Copy link

josacar commented Dec 31, 2022

What about using Rust compiler added for mozilla? https://packages.debian.org/bullseye/rustc-mozilla

@josacar
Copy link

josacar commented Jan 3, 2023

I was able to build ruby 3.2 with yjit only official debian packages:

root@e9078ac731f7:/# cat /etc/debian_version
11.6
root@e9078ac731f7:/# ruby -v --yjit
ruby 3.2.0 (2022-12-25 revision a528908271) +YJIT [x86_64-linux]

This is the patch I used to build it:

diff --git a/Dockerfile.template b/Dockerfile.template
index 880accf..78a2373 100644
--- a/Dockerfile.template
+++ b/Dockerfile.template
@@ -8,7 +8,6 @@
        def should_yjit:
                # https://github.com/docker-library/ruby/issues/390
                ([ "2.7", "3.0", "3.1" ] | index(env.version) | not)
-               and is_alpine
+               and (env.variant | endswith("buster") | not)
 -}}
 {{ if is_alpine then ( -}}
 FROM alpine:{{ env.variant | ltrimstr("alpine") }}
@@ -106,6 +105,14 @@ RUN set -eux; \
 {{ ) else "" end -}}
        ; \
 {{ ) else ( -}}
+{{ if should_yjit then ( -}}
+       yjit=; \
+       debArch="$(uname -m)"; \
+       \
+       case "$debArch" in \
+               x86_64 | aarch64) yjit=1 ;; \
+       esac; \
+{{ ) else "" end -}}
        savedAptMark="$(apt-mark showmanual)"; \
        apt-get update; \
        apt-get install -y --no-install-recommends \
@@ -113,6 +120,10 @@ RUN set -eux; \
                dpkg-dev \
                libgdbm-dev \
                ruby \
+{{ if should_yjit then ( -}}
+               ${yjit:+rustc-mozilla} \
+               ${yjit:+cargo-mozilla} \
+{{ ) else "" end -}}
 {{ if is_slim then ( -}}
                autoconf \
                g++ \
@@ -130,6 +141,13 @@ RUN set -eux; \
 {{ ) else "" end -}}
        ; \
        rm -rf /var/lib/apt/lists/*; \
+{{ if should_yjit then ( -}}
+       \
+       case "$yjit" in \
+               1) cargo --version; \
+                  rustc --version ;; \
+       esac; \
+{{ ) else "" end -}}
 {{ ) end -}}
        \
        wget -O ruby.tar.xz "https://cache.ruby-lang.org/pub/ruby/${RUBY_MAJOR%-rc}/ruby-$RUBY_VERSION.tar.xz"; \

And that used following versions:

+ cargo --version
cargo 1.56.0
+ rustc --version
rustc 1.59.0

Checking the changelog :

  * Disable wasm.
  * Disable new binary packages rustfmt, -clippy, -all.

What do you think about this @tianon ?

@frederikspang
Copy link
Contributor

@josacar Based on discussion here and in #399 - That sounds like a great solution.

Let's await tianon's reply, and I'll be happy to take it for a test-spin here.

@tianon
Copy link
Member

tianon commented Jan 4, 2023

I checked with some cohorts in Debian and they suggested strongly that we shouldn't use rustc-mozilla (or even Debian's rustc package) and that we should use rustup instead. 😞

@BrianHawley
Copy link

I checked with some cohorts in Debian and they suggested strongly that we shouldn't use rustc-mozilla (or even Debian's rustc package) and that we should use rustup instead. 😞

@tianon that sounds like this PR is currently on the right path, as that's what it's doing. Should the alpine images also use rustup? If not, does that mean this PR can be merged? Pending any squashing per policy, if needed, or other required changes.

./rustup-init -y --no-modify-path --profile minimal --default-toolchain $RUST_VERSION --default-host ${rustArch}; \
rm rustup-init; \
cargo --version; \
rustc --version ;; \

Choose a reason for hiding this comment

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

Is it necessary to display the cargo and rustc versions here if you're just going to delete them later?

Copy link
Member

Choose a reason for hiding this comment

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

That's not (necessarily) to display them, but rather to smoke test that the downloaded binaries actually run correctly (so it can error out sooner than it will otherwise, probably deep inside the Ruby build).

@tianon
Copy link
Member

tianon commented Jan 6, 2023

We're still considering which approach we'd prefer to maintain over time -- if we go with this route as-is, we'll end up copying a lot of what https://hub.docker.com/_/rust does, and essentially maintaining it over time, so we're very seriously considering dropping support for mips64le, ppc64le, and s390x on the Debian-based 3.2+ images (which are unsupported by the rust image) and switching to a multi-stage build that can use the rust image more directly.

@BrianHawley
Copy link

BrianHawley commented Jan 6, 2023

We're still considering which approach we'd prefer to maintain over time

@tianon while you're at it, consider that even in cases where YJIT is not supported, the RubyGems cargo builder is, for platforms that rust supports. One advantage of basing things on the rust image is that rust will still be installed for use by gem extensions, without having to be reinstalled. Space is a tradeoff, of course, if you don't need that.

@tianon
Copy link
Member

tianon commented Jan 6, 2023

Interesting! I did some digging, and that would add at least 400MiB+ (probably north of 500MiB, in reality) to our image size, so I don't think we'll likely be keeping Rust in the final images (even in the big "batteries included" variants, because that's a huge size increase even there), but it's good to know. 😅

@tianon
Copy link
Member

tianon commented Jan 6, 2023

Ah, I forgot about "currently supported for macOS and Linux on x86-64 and arm64/aarch64 CPUs" (https://github.com/ruby/ruby/blob/v3_2_0/doc/yjit/yjit.md, https://github.com/ruby/ruby/blob/v3_2_0/configure.ac#L3757-L3761). I guess we don't really have much choice here and do need to reimplement the Rust image to some extent. 😞

@BrianHawley
Copy link

BrianHawley commented Jan 6, 2023

that would add at least 400MiB+ (probably north of 500MiB, in reality) to our image size

Ouch. Maybe people can install rust again if they need to for rust gem extensions, then delete it after the bundle install.

I guess we don't really have much choice here and do need to reimplement the Rust image to some extent. 😞

To be fair, the rust image code is really small and simple, as rustup does most of the work. I do similar tricks to backport the ruby Debian image code to an old Ubuntu LTS, and actually use ERB to generate the Ubuntu file, interpolating the Debian file, in an automated vendoring process. If you use the same environment vars and similar code, the copying and pasting can be automatic.

paihu and others added 2 commits January 9, 2023 10:12
This also creates a separate "rust.json" so we don't update Rust versions automatically but do have a simple way to do so (especially without having to update checksums by hand).
@yosifkit yosifkit merged commit d9b3dc7 into docker-library:master Jan 13, 2023
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Jan 13, 2023
Changes:

- docker-library/ruby@d9b3dc7: Merge pull request docker-library/ruby#400 from paihu/debian-yjit
- docker-library/ruby@c756b06: Refactor to pull Rust version/checksum information from versions.json
- docker-library/ruby@6db728e: Add YJIT support to debian 3.2+
- docker-library/ruby@4edf684: Ditch "tac|tac" for more reliable scraping
@tianon tianon mentioned this pull request May 15, 2023
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.

6 participants