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

Revert #113588 to fix bootstrap timings #114345

Merged
merged 1 commit into from
Aug 2, 2023
Merged

Revert #113588 to fix bootstrap timings #114345

merged 1 commit into from
Aug 2, 2023

Conversation

lqd
Copy link
Member

@lqd lqd commented Aug 1, 2023

This reverts #113588 which seems to have broken perf's bootstrap timings via some git issue

#114318 (comment) show a newly broken benchmark, the error at the time was

fatal: Path 'src/ci/channel' exists on disk, but not in 'e62323df22ecf9c163023132d17b7114f68b72e8'.
       thread 'main' panicked at 'command did not execute successfully: cd "/home/collector/rustc-perf/rust" && "git" "show" "e62323df22ecf9c163023132d17b7114f68b72e8:src/ci/channel"
       expected success, got: exit status: 128', config.rs:1786:27

If this lands, it will reopen #101907 and annoy miri, but it could actually be an issue that would appear during the next bootstrap bump, not just rustc-perf today.

r? @ghost

@lqd
Revert "Rollup merge of rust-lang#113588 - RalfJung:llvm-merge-base, …
7c12b2e 
…r=albertlarsan68"

This reverts commit 849f4f8, reversing
changes made to 0242643.
@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 Aug 1, 2023
@lqd
Copy link
Member Author

lqd commented Aug 1, 2023

Let's see if timings would be fixed
@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 1, 2023
@bors
Copy link
Contributor

bors commented Aug 1, 2023

⌛ Trying commit 7c12b2e with merge 75df98292f8a14c854080a4c2bc5de9a8bed029f...

@bors
Copy link
Contributor

bors commented Aug 1, 2023

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

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (75df98292f8a14c854080a4c2bc5de9a8bed029f): 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)

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

Cycles

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)		 2.5% [2.3%, 2.8%] 4
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: missing data

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 2, 2023
@lqd lqd closed this Aug 2, 2023
@lqd lqd reopened this Aug 2, 2023
RalfJung
RalfJung reviewed Aug 2, 2023
View reviewed changes
src/tools/build_helper/src/git.rs
@@ -78,22 +78,13 @@ pub fn rev_exists(rev: &str, git_dir: Option<&Path>) -> Result<bool, String> {
/// We will then fall back to origin/master in the hope that at least this exists.
pub fn updated_master_branch(git_dir: Option<&Path>) -> Result<String, String> {
let upstream_remote = get_rust_lang_rust_remote(git_dir)?;
for upstream_master in [format!("{upstream_remote}/master"), format!("origin/master")] {
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth checking a revert of just the changes in updated_master_branch? Those are unrelated to the Miri changes, it just seemed reasonable to ensure the remove branch returned here actually exists.

@lqd lqd marked this pull request as ready for review August 2, 2023 17:23
@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2023

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

@lqd
Copy link
Member Author

lqd commented Aug 2, 2023

there is bootstrap data on the server, so let's land this for now, and figure out how to land the initial PR after that

r? @RalfJung

@RalfJung
Copy link
Member

RalfJung commented Aug 2, 2023

I guess let's land this but I hope this doesn't mean #101907 will never get fixed. :(

@bors r+ p=1

@bors
Copy link
Contributor

bors commented Aug 2, 2023

📌 Commit 7c12b2e has been approved by RalfJung

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 Aug 2, 2023
@RalfJung RalfJung mentioned this pull request Aug 2, 2023
Open
@lqd
Copy link
Member Author

lqd commented Aug 2, 2023

If it's just some transient .git state, we should be able to fix it on the server, fix the rustc-perf collector, and reland the PR to fix the issue.

@bors
Copy link
Contributor

bors commented Aug 2, 2023

⌛ Testing commit 7c12b2e with merge 6e6d39a...

@bors
Copy link
Contributor

bors commented Aug 2, 2023

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 6e6d39a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 2, 2023
@bors bors merged commit 6e6d39a into rust-lang:master Aug 2, 2023
22 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 2, 2023
@lqd lqd deleted the revert-113588 branch August 2, 2023 21:21
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6e6d39a): 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)		 2.6% [1.6%, 3.4%] 3
Regressions ❌
(secondary)		 2.3% [1.1%, 3.5%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.0% [-2.0%, -2.0%] 1
All ❌✅ (primary) 2.6% [1.6%, 3.4%] 3

Cycles

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

Binary size

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

Bootstrap: 650.695s -> 648.281s (-0.37%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

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
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
5 participants
@lqd @rust-timer @bors @rustbot @RalfJung