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

Revert #107834 #108302

Merged
merged 2 commits into from
Feb 22, 2023
Merged

Revert #107834 #108302

merged 2 commits into from
Feb 22, 2023

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Feb 21, 2023

This reverts commit 41c6c5d4996728b5a635319ef9b077a3d0ccc480 and #107956. Trying to check if this fixes building rustc for perf bot.

…te symlink for legacy rustfmt path"

This reverts commit 41c6c5d.
@rustbot
Copy link
Collaborator

rustbot commented Feb 21, 2023

r? @albertlarsan68

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Feb 21, 2023
@Kobzol
Copy link
Contributor Author

Kobzol commented Feb 21, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 21, 2023
@bors
Copy link
Contributor

bors commented Feb 21, 2023

⌛ Trying commit a5d2731 with merge 30cee12d335825e29c641d66f7425b8d6329defa...

@lqd
Copy link
Member

lqd commented Feb 21, 2023

This should "fix it" in the sense that rustfmt will no longer be copied into perf.rlo target/ folder. The different reverts have probably caused it to stay copied on the server, so we may want to clean that up. I believe that's why the error changed from being about the other PR to this one.

I also think the reverted PR is buggy anyways so I'll try to fix it.

@Kobzol
Copy link
Contributor Author

Kobzol commented Feb 21, 2023

Good point, the fact that the error has changed is probably because of the file that is now there. In any case, we should find the smallest revert that will get us to a working bootstrap measurement, and then later fix the rustfmt copy issue.

@bors
Copy link
Contributor

bors commented Feb 21, 2023

☀️ Try build successful - checks-actions
Build commit: 30cee12d335825e29c641d66f7425b8d6329defa (30cee12d335825e29c641d66f7425b8d6329defa)

@rust-timer

This comment has been minimized.

@lqd
Copy link
Member

lqd commented Feb 21, 2023

Yeah, but the reverts are unfortunately mixed in with unknown persistent changes to the server and the reverted PR didn't break things by itself it seems, so reverting it would only fix the current state of things as a side-effect of not relying on the dirty folder on the server ? Maybe reverting both is safer. We should probably check the reverts locally to get that info though.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (30cee12d335825e29c641d66f7425b8d6329defa): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.2% [-2.3%, -2.1%] 2
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 21, 2023
@Kobzol
Copy link
Contributor Author

Kobzol commented Feb 21, 2023

@bors try @rust-timer queue

Indeed it looks like both PRs cause problems, at least with the current contents of the target directory on rustc-perf. Let's see if reverting both at once helps.

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 21, 2023
@bors
Copy link
Contributor

bors commented Feb 21, 2023

⌛ Trying commit 6ca499b with merge ecfe2280e3760013c03d8ca5f4165cc7443312f1...

@lqd
Copy link
Member

lqd commented Feb 21, 2023

I believe it should help indeed: the presence of the rustfmt in ./target would now be irrelevant to bootstrap, and avoid the current error.

(I also have a local bootstrap fix for that error)

@bors
Copy link
Contributor

bors commented Feb 21, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ecfe2280e3760013c03d8ca5f4165cc7443312f1): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.4% [1.4%, 1.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.4% [3.2%, 3.6%] 3
Improvements ✅
(primary)
-3.4% [-3.4%, -3.4%] 1
Improvements ✅
(secondary)
-2.4% [-3.3%, -1.1%] 20
All ❌✅ (primary) -3.4% [-3.4%, -3.4%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 21, 2023
@Kobzol
Copy link
Contributor Author

Kobzol commented Feb 21, 2023

It works now. The bootstrap timings are missing, but that's only because the base commit didn't have them. Comparing the last run vs an older nightly (e.g. 2022-02-10) shows bootstrap diff. And the error has also disappeared from the status page.

I propose to merge this and then apply @lqd's fix and then re-apply the stage0 PR, after it's also fixed.

@lqd
Copy link
Member

lqd commented Feb 21, 2023

In theory, reverting #107956 and landing #108316 should also fix it (🤞) -- it does work locally, the symlink won't be recreated if it already exists. If we revert both #107956 and #107834 here, then that fix can be integrated into a future PR without any time pressure, to re-land #107834 in some form.

@Kobzol
Copy link
Contributor Author

Kobzol commented Feb 21, 2023

We know that this works, so I'd merge it ASAP and then land your fix to reapply the rustfmt PR. But both works, I'll let you decide :)

@albertlarsan68
Copy link
Member

I think making sure perf works is the most important.
Cc @jyn514
@bors r+ p=1 rollup=never

@bors
Copy link
Contributor

bors commented Feb 21, 2023

📌 Commit 6ca499b has been approved by albertlarsan68

It is now in the queue for this repository.

@bors 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 Feb 21, 2023
@albertlarsan68
Copy link
Member

@bors p=10

@bors
Copy link
Contributor

bors commented Feb 21, 2023

⌛ Testing commit 6ca499b with merge 375d5ac...

@bors
Copy link
Contributor

bors commented Feb 22, 2023

☀️ Test successful - checks-actions
Approved by: albertlarsan68
Pushing 375d5ac to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 22, 2023
@bors bors merged commit 375d5ac into rust-lang:master Feb 22, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 22, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (375d5ac): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.9% [-2.9%, -2.8%] 4
Improvements ✅
(secondary)
-2.3% [-4.0%, -0.6%] 68
All ❌✅ (primary) -2.9% [-2.9%, -2.8%] 4

Cycles

This benchmark run did not return any relevant results for this metric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants