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

Make Decodable and Decoder infallible. #93066

Merged
merged 3 commits into from
Jan 23, 2022

Conversation

nnethercote
Copy link
Contributor

Decoder has two impls:

  • opaque: this impl is already partly infallible, i.e. in some places it
    currently panics on failure (e.g. if the input is too short, or on a
    bad Result discriminant), and in some places it returns an error
    (e.g. on a bad Option discriminant). The number of places where
    either happens is surprisingly small, just because the binary
    representation has very little redundancy and a lot of input reading
    can occur even on malformed data.
  • json: this impl is fully fallible, but it's only used (a) for the
    .rlink file production, and there's a FIXME comment suggesting it
    should change to a binary format, and (b) in a few tests in
    non-fundamental ways. Indeed Remove all json handling from rustc_serialize #85993 is open to remove it entirely.

And the top-level places in the compiler that call into decoding just
abort on error anyway. So the fallibility is providing little value, and
getting rid of it leads to some non-trivial performance improvements.

Much of this PR is pretty boring and mechanical. Some notes about
a few interesting parts:

  • The commit removes Decoder::{Error,error}.
  • InternIteratorElement::intern_with: the impl for T now has the same
    optimization for small counts that the impl for Result<T, E> has,
    because it's now much hotter.
  • Decodable impls for SmallVec, LinkedList, VecDeque now all use
    collect, which is nice; the one for Vec uses unsafe code, because
    that gave better perf on some benchmarks.

r? @bjorn3

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@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 Jan 19, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 19, 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 Jan 19, 2022
@bors
Copy link
Contributor

bors commented Jan 19, 2022

⌛ Trying commit 197488707c3679e8418fefc5555d3b90f821b59d with merge 88a40e21d3ce57d409eeb40ec75bb9aff99e7836...

compiler/rustc_middle/src/ty/codec.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/context.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jan 19, 2022

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

@rust-timer
Copy link
Collaborator

Queued 88a40e21d3ce57d409eeb40ec75bb9aff99e7836 with parent e5e2b0b, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (88a40e21d3ce57d409eeb40ec75bb9aff99e7836): comparison url.

Summary: This change led to large relevant improvements 🎉 in compiler performance.

  • Large improvement in instruction counts (up to -2.8% on incr-unchanged builds of ctfe-stress-4 check)

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 removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 19, 2022
@nnethercote
Copy link
Contributor Author

Perf results are good, but not as good as I got locally. Instruction counts:

  • CI: 0 regressions, 313 unchanged, 264 improvements; largest improvement 2.83%
  • Local: 2 regressions, 66 unchanged, 276 improvements (I didn't measure doc builds); largest improvement 4.27%

My hypothesis for these differences is still PGO. Particularly for this sort of change which involves many small changes spread out over a lot of code.

@nnethercote
Copy link
Contributor Author

I have updated the code for the requested changes.

Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

r=me with typos fixed

compiler/rustc_middle/src/ty/context.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/context.rs Outdated Show resolved Hide resolved
@nnethercote
Copy link
Contributor Author

@bors r=bjorn3

@bors
Copy link
Contributor

bors commented Jan 20, 2022

📌 Commit 785f0435c21e8a8de83e948d66df76b62e7fa285 has been approved by bjorn3

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

bors commented Jan 21, 2022

☔ The latest upstream changes (presumably #93138) made this pull request unmergeable. Please resolve the merge conflicts.

@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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 21, 2022
Because `()` is called "unit" and it makes it match
`Encoder::emit_unit`.
`Decoder` has two impls:
- opaque: this impl is already partly infallible, i.e. in some places it
  currently panics on failure (e.g. if the input is too short, or on a
  bad `Result` discriminant), and in some places it returns an error
  (e.g. on a bad `Option` discriminant). The number of places where
  either happens is surprisingly small, just because the binary
  representation has very little redundancy and a lot of input reading
  can occur even on malformed data.
- json: this impl is fully fallible, but it's only used (a) for the
  `.rlink` file production, and there's a `FIXME` comment suggesting it
  should change to a binary format, and (b) in a few tests in
  non-fundamental ways. Indeed rust-lang#85993 is open to remove it entirely.

And the top-level places in the compiler that call into decoding just
abort on error anyway. So the fallibility is providing little value, and
getting rid of it leads to some non-trivial performance improvements.

Much of this commit is pretty boring and mechanical. Some notes about
a few interesting parts:
- The commit removes `Decoder::{Error,error}`.
- `InternIteratorElement::intern_with`: the impl for `T` now has the same
  optimization for small counts that the impl for `Result<T, E>` has,
  because it's now much hotter.
- Decodable impls for SmallVec, LinkedList, VecDeque now all use
  `collect`, which is nice; the one for `Vec` uses unsafe code, because
  that gave better perf on some benchmarks.
@nnethercote
Copy link
Contributor Author

I rebased to fix the conflicts.

@bors r=bjorn3

@bors
Copy link
Contributor

bors commented Jan 21, 2022

📌 Commit 37fbd91 has been approved by bjorn3

@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 Jan 21, 2022
@bors
Copy link
Contributor

bors commented Jan 23, 2022

⌛ Testing commit 37fbd91 with merge 84322ef...

@bors
Copy link
Contributor

bors commented Jan 23, 2022

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing 84322ef to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 23, 2022
@bors bors merged commit 84322ef into rust-lang:master Jan 23, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 23, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (84322ef): comparison url.

Summary: This change led to large relevant improvements 🎉 in compiler performance.

  • Large improvement in instruction counts (up to -2.1% on full builds of helloworld doc)

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

@rustbot label: -perf-regression

@nnethercote nnethercote deleted the infallible-decoder branch January 27, 2022 22:19
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 8, 2022
Make `Encodable` and `Encoder` infallible.

A follow-up to rust-lang#93066.

r? `@ghost`
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. 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
Development

Successfully merging this pull request may close these issues.

7 participants