ci: Enable opt-dist for dist-aarch64-linux builds #133807

mrkajetanp · 2024-12-03T16:51:17Z

Enable optimised AArch64 dist builds with the opt-dist pipeline.

For the time being, disable bolt on aarch64 due to upstream bolt bugs.

r? @Kobzol
cc @lqd

try-job: dist-aarch64-linux

rustbot · 2024-12-03T16:51:29Z

Some changes occurred in src/tools/opt-dist

cc @Kobzol

Kobzol · 2024-12-03T16:56:51Z

Hi! Could you please split the part that moves the job to the aarch64 runner and the PGO/LTO part? So that we can evaluate the CI cost of these two actions separately. Thanks!

mrkajetanp · 2024-12-03T17:00:14Z

src/ci/docker/host-aarch64/dist-aarch64-linux/Dockerfile

+ENV SCRIPT python3 ../x.py build --set rust.debug=true opt-dist && \
+      ./build/$HOSTS/stage0-tools-bin/opt-dist linux-ci --  python3 ../x.py dist \
+      --host $HOSTS --target $HOSTS --include-default-paths build-manifest bootstrap


Either way this is a completely new dockerfile, so do you mean just replace this with a simple ./x dist call and then wrap it with opt-dist separately? Just in separate commits or separate PRs altogether?

I meant a separate PR, so that we can land these two changes (move to aarch64 host first, and then enable optimizations) separately :)

Makes sense, just wanted to make sure - no problem :)

lqd · 2024-12-03T17:03:37Z

What improvements are you seeing with this PR, over the current artifacts?

mrkajetanp · 2024-12-03T18:53:12Z

I've not yet benchmarked the changes, and I'm not sure how they compare to the artifacts from cross-compilation because I was only doing aarch64 runs but specifically adding opt-dist with LTO and PGO seems to increase the binary sizes of the main artifacts as follows:

librustc_driver-8c547d00f2ea16d5.so - 84M -> 94M
libLLVM.so.19.1-rust-1.85.0-nightly - 106M -> 108M
rustc -> 2.7M for both, +100 bytes

Kobzol · 2024-12-03T22:25:08Z

@bors try

Let's also see how long it takes with the optimizations.

ci: Enable opt-dist for dist-aarch64-linux builds Move the CI dist-aarch64-linux job to an aarch64 runner and enable optimised dist builds with the opt-dist pipeline. For the time being, disable bolt on aarch64 due to upstream bolt bugs. r? `@Kobzol` cc `@lqd`

bors · 2024-12-03T22:31:37Z

⌛ Trying commit 02b958d with merge b828608...

ci: Enable opt-dist for dist-aarch64-linux builds Move the CI dist-aarch64-linux job to an aarch64 runner and enable optimised dist builds with the opt-dist pipeline. For the time being, disable bolt on aarch64 due to upstream bolt bugs. r? `@Kobzol` cc `@lqd`

bors · 2024-12-03T22:37:32Z

⌛ Trying commit 02b958d with merge 945b9be...

lqd · 2024-12-03T22:43:12Z

That’s not going to be the good try job Jakub :3

bors · 2024-12-03T22:49:29Z

💔 Test failed - checks-actions

Kobzol · 2024-12-03T22:50:25Z

Ah, crap. Thanks!

@bors try

ci: Enable opt-dist for dist-aarch64-linux builds Move the CI dist-aarch64-linux job to an aarch64 runner and enable optimised dist builds with the opt-dist pipeline. For the time being, disable bolt on aarch64 due to upstream bolt bugs. r? `@Kobzol` cc `@lqd` try-job: dist-aarch64-linux

bors · 2024-12-03T22:51:36Z

⌛ Trying commit 02b958d with merge d156b32...

bors · 2024-12-04T02:17:52Z

☀️ Try build successful - checks-actions
Build commit: d156b32 (d156b32c73dd32916f0ca83fb0104e600fbad49d)

mrkajetanp · 2024-12-04T02:33:13Z

So that's an extra hour for LTO+PGO without the cache. 2h22 vs 3h22.

lqd · 2024-12-04T06:12:31Z

@bors try

ci: Enable opt-dist for dist-aarch64-linux builds Move the CI dist-aarch64-linux job to an aarch64 runner and enable optimised dist builds with the opt-dist pipeline. For the time being, disable bolt on aarch64 due to upstream bolt bugs. r? `@Kobzol` cc `@lqd` try-job: dist-aarch64-linux

bors · 2024-12-04T06:34:20Z

⌛ Trying commit 02b958d with merge ef33f24...

bors · 2024-12-04T08:32:45Z

☀️ Try build successful - checks-actions
Build commit: ef33f24 (ef33f249830d94b7afdb529458aae4052f14ca98)

mrkajetanp · 2024-12-04T10:11:57Z

1h54 cached, not so bad. Back to roughly the same time as the x86 cross build then.
Completely outside of the CI costs conversations, these changes not making the overall turnaround time for bors longer than it is already is probably a nice bonus.

lqd · 2024-12-04T10:23:01Z

I assume good benchmark results can also help with the cost discussion.

Kobzol · 2024-12-04T11:16:11Z

Indeed! You can download the CI artifacts e.g. using rustup-toolchain-install-master and benchmark it locally using rustc-perf. It would be nice to see the perf. diff. Let me know on Zulip if you want help with that.

mrkajetanp · 2025-01-14T12:00:03Z

@rustbot ready
Just to clean up the label

Kobzol · 2025-01-14T13:22:12Z

@bors r+

bors · 2025-01-14T13:22:15Z

📌 Commit 3d54764 has been approved by Kobzol

It is now in the queue for this repository.

Rollup of 7 pull requests Successful merges: - rust-lang#132397 (Make missing_abi lint warn-by-default.) - rust-lang#133807 (ci: Enable opt-dist for dist-aarch64-linux builds) - rust-lang#134143 (Convert `struct FromBytesWithNulError` into enum) - rust-lang#134338 (Use a C-safe return type for `__rust_[ui]128_*` overflowing intrinsics) - rust-lang#134678 (Update `ReadDir::next` in `std::sys::pal::unix::fs` to use `&raw const (*p).field` instead of `p.byte_offset().cast()`) - rust-lang#135424 (Detect unstable lint docs that dont enable their feature) - rust-lang#135520 (Make sure we actually use the right trivial lifetime substs when eagerly monomorphizing drop for ADTs) r? `@ghost` `@rustbot` modify labels: rollup

klensy · 2025-01-15T09:42:34Z

r+ but removed S-waiting-on-bors?

Kobzol · 2025-01-15T11:00:12Z

Oops, looks like my manual issue modification has raced with bors :) Thanks!

lqd · 2025-01-15T11:02:37Z

Maybe we should rollup=never big CI changes like these in the future

Kobzol · 2025-01-15T11:04:32Z

Yes, I just thought that after I noticed that it was already included in a rollup 😆 I guess that usually we run perf. for similar changes, so it's done by default, but since we don't have perf. monitoring for ARM (yet!), it wasn't done here, so I forgot about it, sorry.

Just in case the currently running rollup fails, let's mark it as such.

@bors rollup=never

bors · 2025-01-15T12:05:49Z

⌛ Testing commit 3d54764 with merge 2776bdf...

Rollup merge of rust-lang#133807 - mrkajetanp:ci-aarch64-opt-dist, r=Kobzol ci: Enable opt-dist for dist-aarch64-linux builds Move the CI dist-aarch64-linux job to an aarch64 runner and enable optimised dist builds with the opt-dist pipeline. For the time being, disable bolt on aarch64 due to upstream bolt bugs. r? `@Kobzol` cc `@lqd` try-job: dist-aarch64-linux

Kobzol · 2025-01-15T12:07:46Z

^ is hopefully just a visual bug.

saethlin · 2025-01-23T23:29:31Z

src/tools/opt-dist/src/main.rs

+            let is_aarch64 = target_triple.starts_with("aarch64");
+
+            let mut skip_tests = vec![
+                // Fails because of linker errors, as of June 2023.
+                "tests/ui/process/nofile-limit.rs".to_string(),
+            ];
+
+            if is_aarch64 {
+                skip_tests.extend([
+                    // Those tests fail only inside of Docker on aarch64, as of December 2024
+                    "tests/ui/consts/promoted_running_out_of_memory_issue-130687.rs".to_string(),
+                    "tests/ui/consts/large_const_alloc.rs".to_string(),
+                ]);
+            }
+


Please, I beg of you, do not do this. This hack is causing CI to pass and local developer workflows to fail, and it is hiding this regression #135952.

To explain, we already skip some tests in a similar manner on dist x64 (not just the explicitly skipped ones, but also whole test suites, e.g. run-make).

Some tests fail on dist, but work locally, these are candidates for being skipped on CI (after all, running the test suite on an extracted dist atchive is already a hack).

But if the test fails also outside of this extracted dist setup, then it shouldn't be skipped, ofc.

I have the same feedback about the two tests that were using this feature before. See #135961.

Okay, we could remove specific skipped tests, but do you also have an issue with skipping any tests in the dist tests? Because we currently don't run some parts of the test suite, so these are also effectivelly skipped, just without being explicitly enumerated.

Of the 4 tests that were individually skipped, 3 also did not work for me with x test --stage 2 in an aarch64 dev environment, and the 4th was for a platform I don't have a dev environment on.

Ignoring or skipping tests in the harness is fine if there is some fundamental incompatibility with the harness. Or not running them because nobody has gotten to wiring them into opt-dist yet.

I would have preferred these tests be ignored in the tests themselves via annotations; that would have at least not had me chasing my tail wondering how CI could be passing when the tests fail locally. And it would have made the problem more visible.

Yeah I agree with explicit skipping via annotations being the better approach for these kinds of problems, you're right.

I'll write up a patch to change this in a moment, let's see what people say

Good point with the annotations. Recently we added a test annotation for only running a given test in dist jobs, the same should be usable also for ignoring a test in a dist job (#135164 - ignore-dist).

I've posted a PR that I've linked above which fixes the two tests that aren't related to the aarch64 vs x86_64 allocation failure issue. I'm away from a keyboard for a few days, but it looks like it works, and I would recommend applying that change and ignoring the large const allocs failure on aarch64, pending further investigation.

workingjubilee · 2025-01-23T23:47:53Z

Move the CI dist-aarch64-linux job to an aarch64 runner and enable optimised dist builds with the opt-dist pipeline.

We should ideally never simultaneously change the CI runner and also enable an optimized build.

mrkajetanp · 2025-01-24T00:03:00Z

We should ideally never simultaneously change the CI runner and also enable an optimized build.

FWIW the description there is a leftover from before we split the PRs, the runner change was done separately here.

workingjubilee · 2025-01-24T00:40:20Z

Ah, thank you. In that case ideally PRs should be updated so that their messages reflect their actual content, but I am happy to know things happened in a more bisectable fashion.

rustbot assigned Kobzol Dec 3, 2024

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Dec 3, 2024

mrkajetanp commented Dec 3, 2024

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 3, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 14, 2025

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2025

rustbot mentioned this pull request Jan 14, 2025

Tracking issue for release notes of #133807: ci: Enable opt-dist for dist-aarch64-linux builds #135482

Open

3 tasks

Kobzol mentioned this pull request Jan 15, 2025

Tracking issue for speeding up rustc via its build configuration #103595

Open

30 tasks

jhpratt mentioned this pull request Jan 15, 2025

Rollup of 7 pull requests #135525

Merged

Kobzol added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 15, 2025

bors merged commit f9d8b65 into rust-lang:master Jan 15, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Jan 15, 2025

saethlin reviewed Jan 23, 2025

View reviewed changes

adamgemmell mentioned this pull request Jan 24, 2025

Large const allocations no longer lead to graceful errors on aarch64 #135952

Open

ci: Enable opt-dist for dist-aarch64-linux builds #133807

ci: Enable opt-dist for dist-aarch64-linux builds #133807

Conversation

mrkajetanp commented Dec 3, 2024 • edited Loading

rustbot commented Dec 3, 2024

Kobzol commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqd commented Dec 3, 2024

mrkajetanp commented Dec 3, 2024

Kobzol commented Dec 3, 2024

bors commented Dec 3, 2024

bors commented Dec 3, 2024

lqd commented Dec 3, 2024

bors commented Dec 3, 2024

Kobzol commented Dec 3, 2024

bors commented Dec 3, 2024

bors commented Dec 4, 2024

mrkajetanp commented Dec 4, 2024

lqd commented Dec 4, 2024

bors commented Dec 4, 2024

bors commented Dec 4, 2024

mrkajetanp commented Dec 4, 2024

lqd commented Dec 4, 2024

Kobzol commented Dec 4, 2024

mrkajetanp commented Jan 14, 2025

Kobzol commented Jan 14, 2025

bors commented Jan 14, 2025

klensy commented Jan 15, 2025

Kobzol commented Jan 15, 2025

lqd commented Jan 15, 2025

Kobzol commented Jan 15, 2025 • edited Loading

bors commented Jan 15, 2025

Kobzol commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrkajetanp Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

workingjubilee commented Jan 23, 2025

mrkajetanp commented Jan 24, 2025

workingjubilee commented Jan 24, 2025

mrkajetanp commented Dec 3, 2024 •

edited

Loading

Kobzol commented Jan 15, 2025 •

edited

Loading

mrkajetanp Jan 24, 2025 •

edited

Loading

saethlin Jan 24, 2025 •

edited

Loading