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

Port rustc perf cmp command #128868

Merged
merged 2 commits into from
Aug 13, 2024
Merged

Conversation

s7tya
Copy link
Contributor

@s7tya s7tya commented Aug 9, 2024

I've integrated bench_cmp and bench_local into the bootstrap.

r​? @Kobzol

@rustbot
Copy link
Collaborator

rustbot commented Aug 9, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Aug 9, 2024

⚠️ Warning ⚠️

  • These commits modify submodules.

@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 9, 2024
@s7tya
Copy link
Contributor Author

s7tya commented Aug 9, 2024

r? @Kobzol

@rustbot rustbot assigned Kobzol and unassigned Mark-Simulacrum Aug 9, 2024
Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Thank you, looks great! Left a few comments.

src/tools/rustc-perf-wrapper/src/config.rs Outdated Show resolved Hide resolved
src/tools/rustc-perf-wrapper/src/main.rs Show resolved Hide resolved
src/tools/rustc-perf-wrapper/src/main.rs Show resolved Hide resolved
@s7tya s7tya force-pushed the port-rustc-perf-cmp-command branch from 4713ef9 to 83363f2 Compare August 9, 2024 08:43
@s7tya s7tya marked this pull request as ready for review August 9, 2024 08:44
@s7tya
Copy link
Contributor Author

s7tya commented Aug 9, 2024

Thanks! I've fixed them.
(I'm sorry for the mistaken ping. I thought I had specified the reviewer, but it didn't recognize it.)

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 9, 2024
@s7tya s7tya force-pushed the port-rustc-perf-cmp-command branch from 83363f2 to ce1d7d1 Compare August 9, 2024 11:03
Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Awesome, works great!

Idea for improving bench_cmp: if you don't specify IDs, it could show the two latest artifacts in the DB. If you specify only one, it should compare that ID with the latest (different) artifact in the DB.

@Kobzol
Copy link
Contributor

Kobzol commented Aug 9, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 9, 2024

📌 Commit ce1d7d1 has been approved by Kobzol

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 9, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Aug 9, 2024

Actually, this updates rustc-perf, so let's tone back the rolluping.

@bors rollup=never

@s7tya
Copy link
Contributor Author

s7tya commented Aug 9, 2024

if you don't specify IDs, it could show the two latest artifacts in the DB. If you specify only one, it should compare that ID with the latest (different) artifact in the DB.

That's definitely useful! I'm going to impl it.

@bors
Copy link
Contributor

bors commented Aug 13, 2024

⌛ Testing commit ce1d7d1 with merge e9e27ab...

@bors
Copy link
Contributor

bors commented Aug 13, 2024

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing e9e27ab to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 13, 2024
@bors bors merged commit e9e27ab into rust-lang:master Aug 13, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 13, 2024
@s7tya s7tya deleted the port-rustc-perf-cmp-command branch August 13, 2024 04:32
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e9e27ab): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.7% [0.7%, 0.8%] 4
Regressions ❌
(secondary)
0.4% [0.2%, 0.6%] 25
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.7% [0.7%, 0.8%] 4

Max RSS (memory usage)

Results (primary -1.6%, secondary -1.9%)

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.3% [2.1%, 2.5%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-4.2% [-8.4%, -1.3%] 3
Improvements ✅
(secondary)
-1.9% [-1.9%, -1.9%] 2
All ❌✅ (primary) -1.6% [-8.4%, 2.5%] 5

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: 751.195s -> 753.953s (0.37%)
Artifact size: 341.45 MiB -> 341.43 MiB (-0.01%)

@rustbot rustbot added the perf-regression Performance regression. label Aug 13, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Aug 13, 2024

This is most probably caused by small perturbances caused by the rustc-perf update.

@rustbot label: +perf-regression-triaged

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. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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