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

Optimize TyKind::eq. #107717

Merged
merged 1 commit into from
Feb 9, 2023
Merged

Optimize TyKind::eq. #107717

merged 1 commit into from
Feb 9, 2023

Conversation

nnethercote
Copy link
Contributor

r? @ghost

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 6, 2023
@nnethercote
Copy link
Contributor Author

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

bors commented Feb 6, 2023

⌛ Trying commit 1c16de1fa98d9253fdf9431e35c23e2028f4f144 with merge 26022a0f0a676d9f8a79a737e73282e4fc32d886...

@bors
Copy link
Contributor

bors commented Feb 6, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (26022a0f0a676d9f8a79a737e73282e4fc32d886): 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.6% [-0.9%, -0.2%] 11
Improvements ✅
(secondary)
-0.9% [-1.7%, -0.2%] 15
All ❌✅ (primary) -0.6% [-0.9%, -0.2%] 11

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)
-4.0% [-6.4%, -2.5%] 3
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)
2.4% [2.1%, 3.1%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

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

Small but clear icount wins here.

r? @compiler-errors

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

r=me after making tidy happy

@nnethercote
Copy link
Contributor Author

I fixed the assertion text and the formatting that tidy complained about.

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Feb 6, 2023

📌 Commit 1dbed69 has been approved by compiler-errors

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

bors commented Feb 7, 2023

⌛ Testing commit 1dbed69 with merge bc21083bdc3f74597873278a1989cdd59c8d974b...

@bors
Copy link
Contributor

bors commented Feb 7, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 7, 2023
@nnethercote
Copy link
Contributor Author

Some kind of LLVM build failure, presumably unrelated to the changes in this PR:

FAILED: lib/libLLVMMCJIT.a 
cmd.exe /C "cd . && "C:\Program Files\CMake\bin\cmake.exe" -E rm -f lib\libLLVMMCJIT.a && D:\a\rust\rust\mingw64\bin\ar.exe qc lib\libLLVMMCJIT.a  lib/ExecutionEngine/MCJIT/CMakeFiles/LLVMMCJIT.dir/MCJIT.cpp.obj && D:\a\rust\rust\mingw64\bin\ranlib.exe lib\libLLVMMCJIT.a && cd ."
D:\a\rust\rust\mingw64\bin\ranlib.exe: could not create temporary file whilst writing archive: no more archived files

@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-mingw failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[2571/3025] Linking CXX executable bin\FileCheck.exe
[2572/3025] Linking CXX static library lib\libLLVMExecutionEngine.a
[2573/3025] Building CXX object tools/lto/CMakeFiles/LTO.dir/lto.cpp.obj
[2574/3025] Linking CXX static library lib\libLLVMMCJIT.a
FAILED: lib/libLLVMMCJIT.a 
cmd.exe /C "cd . && "C:\Program Files\CMake\bin\cmake.exe" -E rm -f lib\libLLVMMCJIT.a && D:\a\rust\rust\mingw64\bin\ar.exe qc lib\libLLVMMCJIT.a  lib/ExecutionEngine/MCJIT/CMakeFiles/LLVMMCJIT.dir/MCJIT.cpp.obj && D:\a\rust\rust\mingw64\bin\ranlib.exe lib\libLLVMMCJIT.a && cd ."
D:\a\rust\rust\mingw64\bin\ranlib.exe: could not create temporary file whilst writing archive: no more archived files
[2575/3025] Linking CXX static library lib\libLLVMTransformUtils.a
[2576/3025] Linking CXX executable bin\yaml-bench.exe
[2577/3025] Building CXX object tools/llvm-lto/CMakeFiles/llvm-lto.dir/llvm-lto.cpp.obj
[2578/3025] Linking CXX executable bin\llvm-config.exe
[2578/3025] Linking CXX executable bin\llvm-config.exe
[2579/3025] Building CXX object tools/bugpoint/CMakeFiles/bugpoint.dir/BugDriver.cpp.obj
[2580/3025] Building CXX object tools/bugpoint/CMakeFiles/bugpoint.dir/ExecutionDriver.cpp.obj
[2581/3025] Building CXX object tools/llvm-profdata/CMakeFiles/llvm-profdata.dir/llvm-profdata.cpp.obj
ninja: build stopped: subcommand failed.
command did not execute successfully, got: exit code: 1


build script failed, must exit now', C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\cmake-0.1.48\src\lib.rs:975:5
 finished in 267.167 seconds
Build completed successfully in 0:07:42

@nnethercote
Copy link
Contributor Author

@bors retry

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

bors commented Feb 8, 2023

⌛ Testing commit 1dbed69 with merge 8d11058869ad87e2e85ea05c45c664120d3fe927...

@bors
Copy link
Contributor

bors commented Feb 9, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 9, 2023
@nnethercote
Copy link
Contributor Author

On aarch64-gnu it failed, ending with this:

== clock drift check ==
  local time: Thu Feb  9 00:02:07 UTC 2023
Session terminated, killing shell... ...killed.
time="2023-02-09T00:02:08Z" level=error msg="error waiting for container: unexpected EOF"
  network time: 
Error: The operation was canceled.

Again, this seems entirely unrelated to the changes in the PR. Did I just get unlucky again?

@bors retry

@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 9, 2023
@compiler-errors
Copy link
Member

Yeah, bors has been pretty flaky recently imo

@bors
Copy link
Contributor

bors commented Feb 9, 2023

⌛ Testing commit 1dbed69 with merge 575d424...

@bors
Copy link
Contributor

bors commented Feb 9, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 575d424 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 9, 2023
@bors bors merged commit 575d424 into rust-lang:master Feb 9, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 9, 2023
@nnethercote nnethercote deleted the opt-TyKind-eq branch February 9, 2023 03:49
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (575d424): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)
1.1% [1.0%, 1.2%] 2
Regressions ❌
(secondary)
3.6% [3.3%, 4.2%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.5% [-1.6%, -1.3%] 2
All ❌✅ (primary) 1.1% [1.0%, 1.2%] 2

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)
-1.6% [-2.4%, -0.7%] 2
Improvements ✅
(secondary)
-2.8% [-4.7%, -1.9%] 43
All ❌✅ (primary) -1.6% [-2.4%, -0.7%] 2

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)
- - 0
Improvements ✅
(secondary)
-2.2% [-2.2%, -2.2%] 1
All ❌✅ (primary) - - 0

@rustbot rustbot added the perf-regression Performance regression. label Feb 9, 2023
@nnethercote
Copy link
Contributor Author

keccak and cranelift-codegen are noisy. wg-grammar saw the expected benefit, but it's now considered non-significant, I guess it must have been a bit noisy recently as well.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 9, 2023
@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] url test:false 2.955
[RUSTC-TIMING] chrono test:false 2.586
[RUSTC-TIMING] build_script_build test:false 0.753
[RUSTC-TIMING] env_logger test:false 1.818
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.

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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants