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.

Sign up for GitHub

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

Jump to bottom

Compilation time regression in 1.73 when building the fundsp crate #116797

Closed
irh opened this issue Oct 16, 2023 · 6 comments
Closed

Compilation time regression in 1.73 when building the fundsp crate #116797

irh opened this issue Oct 16, 2023 · 6 comments
Assignees
@nnethercote
Labels
C-bug Category: This is a bug. I-compiletime Issue: Problems and improvements with respect to compile times. P-high High priority 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

@irh
Copy link

irh commented Oct 16, 2023

1.73 introduced a regression in compilation time when building the fundsp crate.

Previously, I could expect a clean build of the crate to take ~22s. Now, compilation hangs for several minutes, I haven't left it running longer than that.

I bisected to #114611, and 8378487 appears to be the commit that introduces the slowdown. Reverting the commit on the latest master brings fundsp build times back to normal.
@irh irh added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Oct 16, 2023
@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. 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 16, 2023
@saethlin saethlin added I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 16, 2023
@saethlin
Copy link
Member

I think this is the same root cause as #116624 and #116780.

@Noratrieb
Copy link
Member

@nnethercote

@nnethercote nnethercote self-assigned this Oct 16, 2023
@nnethercote
Copy link
Contributor

nnethercote commented Oct 17, 2023 •
edited
Loading

I think this is the same root cause as #116624 and #116780.

It is the same as #116780. It is not the same as #116624, which is slow even in 1.72.

nnethercote added a commit to nnethercote/rust that referenced this issue Oct 17, 2023
@nnethercote
Fix a performance regression in obligation deduplication.
91f2fbc 
Commit 8378487 from rust-lang#114611 changed the location of an obligation
deduplication step in `opt_normalize_projection_type`. This meant that
deduplication stopped happening on one path where it was still
necessary, causing a couple of drastic performance regressions.

This commit moves the deduplication back to the old location. The good
news is that rust-lang#114611 had four commits and 8378487 was of minimal
importance, so the perf benefits from that PR remain.

Fixes rust-lang#116780, rust-lang#116797.
@nnethercote nnethercote mentioned this issue Oct 17, 2023
Merged
@nnethercote
Copy link
Contributor

Good news: #116826 will fix the regression.

Even better news: on my machine this crate took 3m13s to compile. After removing the 3rd and 4th commits from #114611, it dropped to 22s. After reinstating the 4th commit (because the 3rd commit was the problem) it dropped to 9s :)

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 17, 2023
@bors
Auto merge of rust-lang#116826 - nnethercote:fix-116780-116797, r=com…
39164b8 
…piler-errors

Fix a performance regression in obligation deduplication.

Commit 8378487 from rust-lang#114611 changed the location of an obligation deduplication step in `opt_normalize_projection_type`. This meant that deduplication stopped happening on one path where it was still necessary, causing a couple of drastic performance regressions.

This commit moves the deduplication back to the old location. The good news is that rust-lang#114611 had four commits and 8378487 was of minimal importance, so the perf benefits from that PR remain.

Fixes rust-lang#116780, rust-lang#116797.

r? `@compiler-errors`
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@irh in the next day a new nightly with a fix should be out. If you have a chance, feel free to test it and confirm if it fixes for you too. thanks for reporting the issue.

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 17, 2023
@irh
Copy link
Author

irh commented Oct 17, 2023

@apiraino I tested it on master, and can confirm it's fixed.

@nnethercote Thanks for taking a look at this so quickly! I also get the 9s build time, very nice 🥳

@irh irh closed this as completed Oct 17, 2023
cuviper pushed a commit to cuviper/rust that referenced this issue Oct 21, 2023
@nnethercote @cuviper
Fix a performance regression in obligation deduplication.
e45b2ab 
Commit 8378487 from rust-lang#114611 changed the location of an obligation
deduplication step in `opt_normalize_projection_type`. This meant that
deduplication stopped happening on one path where it was still
necessary, causing a couple of drastic performance regressions.

This commit moves the deduplication back to the old location. The good
news is that rust-lang#114611 had four commits and 8378487 was of minimal
importance, so the perf benefits from that PR remain.

Fixes rust-lang#116780, rust-lang#116797.

(cherry picked from commit 91f2fbc)
xobs pushed a commit to betrusted-io/rust that referenced this issue Nov 7, 2023
@nnethercote @xobs
Fix a performance regression in obligation deduplication.
4c448dc 
Commit 8378487 from rust-lang#114611 changed the location of an obligation
deduplication step in `opt_normalize_projection_type`. This meant that
deduplication stopped happening on one path where it was still
necessary, causing a couple of drastic performance regressions.

This commit moves the deduplication back to the old location. The good
news is that rust-lang#114611 had four commits and 8378487 was of minimal
importance, so the perf benefits from that PR remain.

Fixes rust-lang#116780, rust-lang#116797.

(cherry picked from commit 91f2fbc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nnethercote nnethercote

Labels
C-bug Category: This is a bug. I-compiletime Issue: Problems and improvements with respect to compile times. P-high High priority 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
Milestone
No milestone
Development

No branches or pull requests
6 participants
@irh @nnethercote @apiraino @saethlin @rustbot @Noratrieb