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

Use the correct llvm-profdata binary in opt-dist #114344

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Use the correct llvm-profdata binary in opt-dist #114344

merged 1 commit into from
Aug 7, 2023

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Aug 1, 2023

Turns out that we were probably using the wrong llvm-profdata binary in the PGO script all along. This should resolve the performance regressions of switching the host LLVM to 17 (host llvm-profdata, target llvm-profdata]).

r? @nikic

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 1, 2023
@Kobzol
Generalize duration analysis
acb617c 
Use the correct `llvm-profdata` binary in `opt-dist`
@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 2, 2023

@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 2, 2023
@bors
Copy link
Contributor

bors commented Aug 2, 2023

⌛ Trying commit acb617c with merge 5e93bd21fa944a163ae2b92f058b2ea8f4622840...

@bors
Copy link
Contributor

bors commented Aug 2, 2023

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

@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 2, 2023

@rust-timer build 5e93bd21fa944a163ae2b92f058b2ea8f4622840

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5e93bd21fa944a163ae2b92f058b2ea8f4622840): comparison URL.

Overall result: ✅ improvements - 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)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.6% [-0.6%, -0.6%] 1
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.7% [3.7%, 3.7%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

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.605s -> 647.928s (-0.41%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 2, 2023
@nikic
Copy link
Contributor

nikic commented Aug 7, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 7, 2023

📌 Commit acb617c has been approved by nikic

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 7, 2023
@bors
Copy link
Contributor

bors commented Aug 7, 2023

⌛ Testing commit acb617c with merge 723895ee45f5edbdb3c350bb49db602ad627a467...

@bors
Copy link
Contributor

bors commented Aug 7, 2023

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 723895ee45f5edbdb3c350bb49db602ad627a467 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 7, 2023
@bors
Copy link
Contributor

bors commented Aug 7, 2023

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 7, 2023

@bors retry

@Kobzol Kobzol closed this Aug 7, 2023
@Kobzol Kobzol reopened this Aug 7, 2023
@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 7, 2023

@bors retry

@lqd
Copy link
Member

lqd commented Aug 7, 2023

@bors r=nikic

@bors
Copy link
Contributor

bors commented Aug 7, 2023

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Aug 7, 2023

📌 Commit acb617c has been approved by nikic

It is now in the queue for this repository.

@lqd
Copy link
Member

lqd commented Aug 7, 2023

💡 This pull request was already approved, no need to approve it again.

I know, bors, I know.

@bors
Copy link
Contributor

bors commented Aug 7, 2023

⌛ Testing commit acb617c with merge 03a119b...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (723895ee45f5edbdb3c350bb49db602ad627a467): 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)		 1.9% [1.8%, 2.2%] 3
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -3.0% [-4.1%, -1.8%] 2
All ❌✅ (primary) - - 0

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)		 - - 0
Improvements ✅
(primary)		 -4.8% [-5.3%, -4.2%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -4.8% [-5.3%, -4.2%] 2

Binary size

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

Bootstrap: 652.069s -> 650.598s (-0.23%)

@bors
Copy link
Contributor

bors commented Aug 7, 2023

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 03a119b to master...

@bors bors merged commit 03a119b into rust-lang:master Aug 7, 2023
23 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 7, 2023
@Kobzol Kobzol deleted the opt-dist-llvm-profdata branch August 7, 2023 21:08
@Kobzol Kobzol mentioned this pull request Aug 7, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 8, 2023
@matthiaskrgr
Rollup merge of rust-lang#114596 - Kobzol:opt-dist-host, r=lqd
07b2c97 
Rename method in `opt-dist`

This makes it clearer that the LLVM is the host one (it doesn't necessarily have to be downloaded). On Linux, it comes from the Dockerfile, on Windows it's downloaded.

Suggested here: rust-lang#114344 (comment)

r? `@lqd`
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (03a119b): 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)		 5.6% [5.6%, 5.6%] 1
Regressions ❌
(secondary)		 4.7% [4.7%, 4.7%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 5.6% [5.6%, 5.6%] 1

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: 651.258s -> 652.752s (0.23%)

@lqd lqd mentioned this pull request Apr 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic left review comments

@lqd lqd lqd left review comments

Assignees

@nikic nikic

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.
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Kobzol @rust-timer @bors @nikic @lqd @rustbot