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

incr. comp.: Don't export impl_stable_hash_via_hash!() and warn about using it. #96082

Merged
merged 1 commit into from
Apr 20, 2022
Merged

incr. comp.: Don't export impl_stable_hash_via_hash!() and warn about using it. #96082

merged 1 commit into from
Apr 20, 2022

Conversation

michaelwoerister
Copy link
Member

Fixes #96013.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 15, 2022
@rust-highfive
Copy link
Collaborator

r? @compiler-errors

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 15, 2022
@michaelwoerister
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

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

bors commented Apr 15, 2022

⌛ Trying commit adf5e92d955f9743dc0a1b7f910325a07f12d2a3 with merge 5dbae17b7833eda353b535611e10d4d0e9755782...

@bors
Copy link
Contributor

bors commented Apr 15, 2022

💔 Test failed - checks-actions

@bors bors 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 Apr 15, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@michaelwoerister
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Apr 15, 2022

⌛ Trying commit c4c617967ab8d5e31b3a40e3f9d060714a92828d with merge bf081a0cd1b0ce034bb940f1a05211303858209f...

@bors
Copy link
Contributor

bors commented Apr 15, 2022

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

@rust-timer
Copy link
Collaborator

Queued bf081a0cd1b0ce034bb940f1a05211303858209f with parent 1e6fe58, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bf081a0cd1b0ce034bb940f1a05211303858209f): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 6 1 0 1
mean2 N/A 0.4% -0.8% N/A -0.8%
max N/A 0.4% -0.8% N/A -0.8%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 15, 2022
@michaelwoerister
Copy link
Member Author

Removed the #[inline] annotations on the non-trivial hash_stable() methods. Let's see if that gets rid of the performance regressions.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

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

bors commented Apr 19, 2022

⌛ Trying commit c0be619 with merge ea7eb7d2194a3cffe4b2b3529e8d4a0bf36e0159...

@bors
Copy link
Contributor

bors commented Apr 19, 2022

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

@rust-timer
Copy link
Collaborator

Queued ea7eb7d2194a3cffe4b2b3529e8d4a0bf36e0159 with parent e2661ba, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ea7eb7d2194a3cffe4b2b3529e8d4a0bf36e0159): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 0 9 9 9
mean2 N/A N/A -0.3% -0.5% -0.3%
max N/A N/A -0.4% -0.7% -0.4%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 19, 2022
@michaelwoerister michaelwoerister removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 19, 2022
@michaelwoerister
Copy link
Member Author

This looks better. The PR should be ready for review now.

@compiler-errors
Copy link
Member

Looks good

@bors r+

@bors
Copy link
Contributor

bors commented Apr 20, 2022

📌 Commit c0be619 has been approved by compiler-errors

@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 Apr 20, 2022
@bors
Copy link
Contributor

bors commented Apr 20, 2022

⌛ Testing commit c0be619 with merge 27af517...

@bors
Copy link
Contributor

bors commented Apr 20, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 20, 2022
@bors bors merged commit 27af517 into rust-lang:master Apr 20, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 20, 2022
@bors bors mentioned this pull request Apr 20, 2022
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (27af517): comparison url.

Summary:

  • Primary benchmarks: mixed results
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 1 0 23 9 24
mean2 3.1% N/A -0.4% -0.5% -0.2%
max 3.1% N/A -0.6% -0.7% 3.1%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot added the perf-regression Performance regression. label Apr 20, 2022
@michaelwoerister
Copy link
Member Author

Hm, that regression is unexpected and did not show up in previous perf run. Could it be spurious?

@compiler-errors
Copy link
Member

It looks spurious to me...

@michaelwoerister
Copy link
Member Author

Yes, the syn-1.0.89 benchmark looks flaky at the moment and the regression has gone away in subsequent perf runs.
@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 21, 2022
@rylev
Copy link
Member

rylev commented Apr 26, 2022

Linking with rust-lang/rustc-perf#1301 which tracks the issue of syn-1.0.89 opt being flaky.

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

@compiler-errors compiler-errors compiler-errors left review comments

Assignees

@compiler-errors compiler-errors

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

Successfully merging this pull request may close these issues.

Remove impl_stable_hash_via_hash!()
8 participants
@michaelwoerister @rust-highfive @rust-timer @bors @rust-log-analyzer @compiler-errors @rylev @rustbot