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

Make Encodable and Encoder infallible. #94732

Merged
merged 5 commits into from
Jun 8, 2022
Merged

Make Encodable and Encoder infallible. #94732

merged 5 commits into from
Jun 8, 2022

Conversation

nnethercote
Copy link
Contributor

A follow-up to #93066.

r? @ghost

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 8, 2022
@nnethercote nnethercote marked this pull request as draft March 8, 2022 08:51
@nnethercote
Copy link
Contributor 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 Mar 8, 2022
@bors
Copy link
Contributor

bors commented Mar 8, 2022

⌛ Trying commit 9e1eb27b5c6a145f2f91c64d132efdb55e0acdc2 with merge 95a1107bff47aa33e13784202ebb1ab616f6e698...

@bors
Copy link
Contributor

bors commented Mar 8, 2022

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

@rust-timer
Copy link
Collaborator

Queued 95a1107bff47aa33e13784202ebb1ab616f6e698 with parent 67b3e81, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (95a1107bff47aa33e13784202ebb1ab616f6e698): comparison url.

Summary: This benchmark run shows 41 relevant improvements 🎉 to instruction counts.

  • Arithmetic mean of relevant improvements: -0.3%
  • Arithmetic mean of all relevant changes: -0.3%
  • Largest improvement in instruction counts: -0.6% on incr-unchanged builds of ucd opt

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 led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@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 Mar 8, 2022
@nnethercote
Copy link
Contributor 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 Jun 6, 2022
@bors
Copy link
Contributor

bors commented Jun 6, 2022

⌛ Trying commit 5c9539014954117f204b6ec98cfc9a948cb4fb54 with merge d0cd2938533ad693c7fc03067773dec8bf4728d4...

@bors
Copy link
Contributor

bors commented Jun 6, 2022

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

@rust-timer
Copy link
Collaborator

Queued d0cd2938533ad693c7fc03067773dec8bf4728d4 with parent e70c60d, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d0cd2938533ad693c7fc03067773dec8bf4728d4): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 -0.4% -0.6% 74
Improvements 🎉
(secondary)		 -0.4% -0.5% 25
All 😿🎉 (primary) -0.4% -0.6% 74

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 3.8% 3.8% 1
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -2.4% -2.4% 1
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 2.3% 2.5% 2
Regressions 😿
(secondary)		 2.4% 2.4% 1
Improvements 🎉
(primary)		 -2.8% -2.8% 1
Improvements 🎉
(secondary)		 -3.2% -4.1% 4
All 😿🎉 (primary) 0.6% -2.8% 3

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. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

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

This isn't the most compelling PR I've ever written, but I think it's worth merging.

Pros:

  • Slight perf improvements on incremental builds
  • Slightly more concise code in a lot of places

Cons:

  • Non-standard error handling for encoders

r? @bjorn3

Best reviewed one commit at a time.

@nnethercote
Copy link
Contributor 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 Jun 7, 2022
@bors
Copy link
Contributor

bors commented Jun 7, 2022

⌛ Trying commit a81c7f05b6356a8a660c5a9ce991cadec82c86f7 with merge 4fea8241b7f680ccdb8fa6e05ed2ce503bd6c1ba...

@bors
Copy link
Contributor

bors commented Jun 8, 2022

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing 1a97162 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 8, 2022
@bors bors merged commit 1a97162 into rust-lang:master Jun 8, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 8, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1a97162): comparison url.

Instruction count

  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 0.4% 0.8% 16
Regressions 😿
(secondary)		 0.5% 0.9% 15
Improvements 🎉
(primary)		 -0.3% -0.5% 25
Improvements 🎉
(secondary)		 -0.4% -0.5% 20
All 😿🎉 (primary) -0.1% 0.8% 41

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 -1.8% -1.8% 1
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) -1.8% -1.8% 1

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -2.6% -2.7% 2
All 😿🎉 (primary) N/A N/A 0

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. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot added the perf-regression Performance regression. label Jun 8, 2022
@bjorn3
Copy link
Member

bjorn3 commented Jun 8, 2022

@nnethercote This is worse than the previous run. On average it now seems to not have much effect at all.

@nnethercote nnethercote deleted the infallible-encoder branch June 8, 2022 23:16
@nnethercote
Copy link
Contributor Author

Sigh. I have no idea what happened here. I did three perf runs before the merge on different versions. They were all uniform improvements on instruction counts, and there wasn't much difference between the various versions. And clearly less work is being done than before, because there's so much less error checking.

@nnethercote nnethercote mentioned this pull request Jun 9, 2022
Merged
@nnethercote
Copy link
Contributor Author

I have filed #97905 to performance test a reversion of this PR, to see what happens.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 11, 2022
@bors
Auto merge of rust-lang#97905 - nnethercote:revert-infallible-encoder…
c845946 
…, r=bjorn3

Revert part of rust-lang#94372 to improve performance

rust-lang#94732 was supposed to give small but widespread performance improvements, as judged from three per-merge performance runs. But the performance run that occurred after merging included a roughly equal number of improvements and regressions, for unclear reasons.

This PR is for a test run reverting those changes, to see what happens.

r? `@ghost`
@nnethercote
Copy link
Contributor Author

Good news: #97905 fixed the regressions here. That PR plus this PR combined gave a clear performance win.

nnethercote added a commit to nnethercote/rust that referenced this pull request Jun 14, 2022
@nnethercote
Rename rustc_serialize::opaque::Encoder as MemEncoder.
abe45a9 
This avoids the name clash with `rustc_serialize::Encoder` (a trait),
and allows lots qualifiers to be removed and imports to be simplified
(e.g. fewer `as` imports).

(This was previously merged as commit 5 in rust-lang#94732 and then was reverted
in rust-lang#97905 because of a perf regression caused by commit 4 in rust-lang#94732.)
@nnethercote nnethercote mentioned this pull request Jun 14, 2022
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 14, 2022
@Dylan-DPC
Rollup merge of rust-lang#98083 - nnethercote:rename-Encoder, r=bjorn3
3222d7c 
Rename rustc_serialize::opaque::Encoder as MemEncoder.

This avoids the name clash with `rustc_serialize::Encoder` (a trait),
and allows lots qualifiers to be removed and imports to be simplified
(e.g. fewer `as` imports).

(This was previously merged as commit 5 in rust-lang#94732 and then was reverted
in rust-lang#97905 because of a perf regression caused by commit 4 in rust-lang#94732.)

r? `@bjorn3`
@pnkfelix
Copy link
Member

Good news: #97905 fixed the regressions here. That PR plus this PR combined gave a clear performance win.

Thanks @nnethercote !

@rustbot label: perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jun 14, 2022
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 14, 2022
@JohnTitor
Rollup merge of rust-lang#98083 - nnethercote:rename-Encoder, r=bjorn3
1d4822d 
Rename rustc_serialize::opaque::Encoder as MemEncoder.

This avoids the name clash with `rustc_serialize::Encoder` (a trait),
and allows lots qualifiers to be removed and imports to be simplified
(e.g. fewer `as` imports).

(This was previously merged as commit 5 in rust-lang#94732 and then was reverted
in rust-lang#97905 because of a perf regression caused by commit 4 in rust-lang#94732.)

r? ``@bjorn3``
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 15, 2022
@JohnTitor
Rollup merge of rust-lang#98083 - nnethercote:rename-Encoder, r=bjorn3
97b9347 
Rename rustc_serialize::opaque::Encoder as MemEncoder.

This avoids the name clash with `rustc_serialize::Encoder` (a trait),
and allows lots qualifiers to be removed and imports to be simplified
(e.g. fewer `as` imports).

(This was previously merged as commit 5 in rust-lang#94732 and then was reverted
in rust-lang#97905 because of a perf regression caused by commit 4 in rust-lang#94732.)

r? ```@bjorn3```
nnethercote added a commit to nnethercote/rust that referenced this pull request Jun 16, 2022
@nnethercote
Move finish out of the Encoder trait.
bb02cc4 
This simplifies things, but requires making `CacheEncoder` non-generic.

(This was previously merged as commit 4 in rust-lang#94732 and then was reverted
in rust-lang#97905 because it caused a perf regression.)
@nnethercote nnethercote mentioned this pull request Jun 16, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 17, 2022
@bors
Auto merge of rust-lang#98160 - nnethercote:mv-finish-out-of-Encoder,…
0423e06 
… r=bjorn3

Move `finish` out of the `Encoder` trait.

This simplifies things, but requires making `CacheEncoder` non-generic.

(This was previously merged as commit 4 in rust-lang#94732 and then was reverted
in rust-lang#97905 because it caused a perf regression.)

r? `@ghost`
@saethlin saethlin mentioned this pull request Sep 4, 2023
Closed
@saethlin saethlin mentioned this pull request Oct 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 approved these changes

Assignees

@bjorn3 bjorn3

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

Successfully merging this pull request may close these issues.

None yet
7 participants
@nnethercote @rust-timer @bors @rust-log-analyzer @bjorn3 @pnkfelix @rustbot