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

regression: crate compiles much slower with 1.82 #132064

Open
Nutomic opened this issue Oct 23, 2024 · 12 comments · Fixed by #132625
Open

regression: crate compiles much slower with 1.82 #132064

Nutomic opened this issue Oct 23, 2024 · 12 comments · Fixed by #132625
Labels
C-bug Category: This is a bug. I-compiletime Issue: Problems and improvements with respect to compile times. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Nutomic
Copy link

Nutomic commented Oct 23, 2024

Our project takes much longer to compile with Rust 1.82 compared to previous versions. To reproduce, git clone https://github.com/LemmyNet/lemmy.git and run cargo check (also happens with build or lint). The problem happens specifically with the crate lemmy_db_views.

cargo check with 1.82: 6m 19s

cargo check with 1.81: 1m 22s

Version it worked on

It most recently worked on:1.81

Version with regression

rustc --version --verbose:

cargo 1.82.0 (8f40fc59f 2024-08-21)
release: 1.82.0
commit-hash: 8f40fc59fb0c8df91c97405785197f3c630304ea
commit-date: 2024-08-21
host: x86_64-unknown-linux-gnu
libgit2: 1.8.1 (sys:0.19.0 vendored)
libcurl: 8.9.0-DEV (sys:0.4.74+curl-8.9.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Manjaro 24.1.1 (Xahea) [64-bit]

@rustbot modify labels: +regression-from-stable-to-1.82 -regression-untriaged

@Nutomic Nutomic added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Oct 23, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 23, 2024
@lqd
Copy link
Member

lqd commented Oct 23, 2024

This is another regression from #126024

@Nothing4You
Copy link

some more information:

1.81

$ rustup override set 1.81
info: override toolchain for '/path/to/lemmy' set to '1.81-aarch64-apple-darwin'
$ rustc --version --verbose
rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: aarch64-apple-darwin
release: 1.81.0
LLVM version: 18.1.7
$ rm -rf target && cargo build --timings
[…]
    Finished `dev` profile [unoptimized] target(s) in 2m 54s

1.82

$ rustup override set 1.82
info: override toolchain for '/path/to/lemmy' set to '1.82-aarch64-apple-darwin'
$ rustc --version --verbose
rustc 1.82.0 (f6e511eec 2024-10-15)
binary: rustc
commit-hash: f6e511eec7342f59a25f7c0534f1dbea00d01b14
commit-date: 2024-10-15
host: aarch64-apple-darwin
release: 1.82.0
LLVM version: 19.1.1
$ rm -rf target && cargo build --timings
[…]
    Finished `dev` profile [unoptimized] target(s) in 9m 58s

cargo-timings.zip

@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 23, 2024
@workingjubilee workingjubilee added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-untriaged Untriaged performance or correctness regression. labels Oct 24, 2024
@bors bors closed this as completed in c07aa1e Nov 7, 2024
@lqd lqd reopened this Nov 7, 2024
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Nov 7, 2024
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-compiletime Issue: Problems and improvements with respect to compile times. labels Nov 7, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Nov 7, 2024

Triage notes (AFAIUI): #132625 is merged, but the compile time is not fully clawed back as #132625 is a compromise between (full) soundness and performance in favor of a full revert (full revert would bring back more soundness problems AFAICT)

This refines the check implemented in #126024 to only disable the global cache if the predicate being considered has opaques in it. This is still theoretically unsound, since goals can indirectly rely on opaques in the defining scope, but we're much less likely to hit it.

It doesn't totally fix #132064: for example, lemmy goes from 1:29 (on rust 1.81) to 9:53 (on nightly) to 4:07 (after this PR). But I think it's at least more sound than a total revert :/

Future improvements might be possible to further claw back the compile times, but should at least be better for now with #132625.

(Please feel free to edit this message to better reflect the status)

@sunshowers
Copy link
Contributor

sunshowers commented Nov 8, 2024

Chiming in -- at Oxide, in our largest repository, this appears to have made many operations much slower. For example, cargo check is twice as slow:

1.81: 3m 37s
1.82: 6m 51s

The slowdown is isolated to Diesel-generated code.

We've pinned to 1.81 for now.

@lqd
Copy link
Member

lqd commented Nov 8, 2024

Can you test on nightly? Things should be better with #132625.

@sunshowers
Copy link
Contributor

sunshowers commented Nov 8, 2024

On my machine, cargo check --all-targets against Omicron 3f6cd90d6a0fe523349e52e6df0554f67167130f:

Rust 1.81.0:
853.39s user 143.94s system 966% cpu 1:43.15 total

Rust 1.84.0-nightly (b91a3a056 2024-11-07):
847.84s user 142.64s system 955% cpu 1:43.70 total

That looks much better. Thanks!

@sunshowers
Copy link
Contributor

(Not clear to me why it looks like the regression has been fully addressed for us, but not for Lemmy.)

@sunshowers
Copy link
Contributor

Ah, okay, some commands are still slower. With the same revision, cargo build -p omicron-nexus:

Rust 1.81:
664.65s user 95.12s system 600% cpu 2:06.44 total

Rust 1.84.0-nightly (b91a3a056 2024-11-07):
641.80s user 90.41s system 542% cpu 2:15.03 total

I don't have the time at the moment to dig into exactly what's causing the slowdown. Our Diesel-generated code is already the slowest to compile, so any regression to it is pretty rough for iteration cycles.

@Nothing4You
Copy link

just did another cargo build test with lemmy on macos arm64:

rustc 1.81.0 (eeb90cd 2024-09-04):

Finished dev profile [unoptimized] target(s) in 2m 49s

rustc 1.84.0-nightly (b91a3a0 2024-11-07):

Finished dev profile [unoptimized] target(s) in 3m 04s

build times for lemmy seem to be fine again for me as well.

sunshowers added a commit to oxidecomputer/omicron that referenced this issue Nov 8, 2024
Rust 1.82 has a rather serious compile time regression in Diesel-generated
code: rust-lang/rust#132064. This makes Omicron
builds up to twice as slow.

We used to be on 1.80 before moving to 1.82, but 1.81 has some nice
things like `#[expect]`. So move to 1.81.

We expect the regression to be fixed in Rust 1.84.

Thanks to @jmpesp for tracking this down!
@compiler-errors
Copy link
Member

Ah, okay, some commands are still slower. With the same revision, cargo build -p omicron-nexus:

Rust 1.81:
664.65s user 95.12s system 600% cpu 2:06.44 total

Rust 1.84.0-nightly (b91a3a056 2024-11-07):
641.80s user 90.41s system 542% cpu 2:15.03 total

I don't have the time at the moment to dig into exactly what's causing the slowdown. Our Diesel-generated code is already the slowest to compile, so any regression to it is pretty rough for iteration cycles.

Hey @sunshowers, I just wanted to let you know that while I'm glad your build times are back down from their 2x build time regressions, we're probably not going to be able to recover that final ~9 seconds that you noted in your comment for omicron-nexus (2:06 -> 2:15) anytime soon, at least not without reintroducing unsoundness into the trait system or spending a disproportionate amount of time to investigate caching improvements to the old trait solver (which is already on its way out, and pretty hard to improve today).

Sorry, and I hope that it doesn't hinder your builds too much! I'll nominate my fix PR for 1.83 backport so it may get to y'all sooner.

@sunshowers
Copy link
Contributor

Thanks @compiler-errors. Looking forward to the new trait solver!

@jieyouxu
Copy link
Member

jieyouxu commented Dec 10, 2024

Looks accidental due to magic comment

@jieyouxu jieyouxu reopened this Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-compiletime Issue: Problems and improvements with respect to compile times. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
10 participants