Make `Decodable` and `Decoder` infallible. #93066

nnethercote · 2022-01-19T06:31:02Z

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 · 2022-01-19T06:31:05Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

nnethercote · 2022-01-19T06:31:23Z

@bors try @rust-timer queue

rust-timer · 2022-01-19T06:31:24Z

Awaiting bors try build completion.

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

bors · 2022-01-19T06:31:32Z

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

compiler/rustc_middle/src/ty/codec.rs

compiler/rustc_middle/src/ty/context.rs

bors · 2022-01-19T08:08:12Z

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

rust-timer · 2022-01-19T08:08:14Z

Queued 88a40e21d3ce57d409eeb40ec75bb9aff99e7836 with parent e5e2b0b, future comparison URL.

rust-timer · 2022-01-19T09:43:17Z

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

nnethercote · 2022-01-19T22:53:30Z

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 · 2022-01-19T23:10:13Z

I have updated the code for the requested changes.

bjorn3

r=me with typos fixed

compiler/rustc_middle/src/ty/context.rs

nnethercote · 2022-01-20T00:43:16Z

@bors r=bjorn3

bors · 2022-01-20T00:43:18Z

📌 Commit 785f0435c21e8a8de83e948d66df76b62e7fa285 has been approved by bjorn3

bors · 2022-01-21T07:06:28Z

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

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 · 2022-01-21T23:43:09Z

I rebased to fix the conflicts.

@bors r=bjorn3

bors · 2022-01-21T23:43:10Z

📌 Commit 37fbd91 has been approved by bjorn3

bors · 2022-01-23T15:37:47Z

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

bors · 2022-01-23T18:51:01Z

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

rust-timer · 2022-01-23T20:22:36Z

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

Make `Encodable` and `Encoder` infallible. A follow-up to rust-lang#93066. r? `@ghost`

rust-highfive assigned bjorn3 Jan 19, 2022

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

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 19, 2022

bjorn3 requested changes Jan 19, 2022

View reviewed changes

compiler/rustc_middle/src/ty/codec.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/ty/context.rs Outdated Show resolved Hide resolved

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 19, 2022

bjorn3 approved these changes Jan 19, 2022

View reviewed changes

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 force-pushed the infallible-decoder branch from 9a52d4c to 785f043 Compare January 20, 2022 00:42

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 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

nnethercote added 3 commits January 22, 2022 10:22

Rename Decoder::read_nil and read_unit.

88600a6

Because `()` is called "unit" and it makes it match `Encoder::emit_unit`.

Address review comments.

37fbd91

nnethercote force-pushed the infallible-decoder branch from 785f043 to 37fbd91 Compare January 21, 2022 23:42

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 added the merged-by-bors This PR was explicitly merged by bors. label Jan 23, 2022

bors merged commit 84322ef into rust-lang:master Jan 23, 2022

rustbot added this to the 1.60.0 milestone Jan 23, 2022

nnethercote deleted the infallible-decoder branch January 27, 2022 22:19

CAD97 mentioned this pull request Feb 2, 2022

Load-bearing comment Incremental ICE #93602

Closed

nnethercote mentioned this pull request Mar 8, 2022

Make Encodable and Encoder infallible. #94732

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 8, 2022

Auto merge of rust-lang#94732 - nnethercote:infallible-encoder, r=bjorn3

1a97162

Make `Encodable` and `Encoder` infallible. A follow-up to rust-lang#93066. r? `@ghost`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Decodable` and `Decoder` infallible. #93066

Make `Decodable` and `Decoder` infallible. #93066

nnethercote commented Jan 19, 2022

rust-highfive commented Jan 19, 2022

nnethercote commented Jan 19, 2022

rust-timer commented Jan 19, 2022

bors commented Jan 19, 2022

bors commented Jan 19, 2022

rust-timer commented Jan 19, 2022

rust-timer commented Jan 19, 2022

nnethercote commented Jan 19, 2022

nnethercote commented Jan 19, 2022

bjorn3 left a comment

nnethercote commented Jan 20, 2022

bors commented Jan 20, 2022

bors commented Jan 21, 2022

nnethercote commented Jan 21, 2022

bors commented Jan 21, 2022

bors commented Jan 23, 2022

bors commented Jan 23, 2022

rust-timer commented Jan 23, 2022

Make Decodable and Decoder infallible. #93066

Make Decodable and Decoder infallible. #93066

Conversation

nnethercote commented Jan 19, 2022

rust-highfive commented Jan 19, 2022

nnethercote commented Jan 19, 2022

rust-timer commented Jan 19, 2022

bors commented Jan 19, 2022

bors commented Jan 19, 2022

rust-timer commented Jan 19, 2022

rust-timer commented Jan 19, 2022

nnethercote commented Jan 19, 2022

nnethercote commented Jan 19, 2022

bjorn3 left a comment

Choose a reason for hiding this comment

nnethercote commented Jan 20, 2022

bors commented Jan 20, 2022

bors commented Jan 21, 2022

nnethercote commented Jan 21, 2022

bors commented Jan 21, 2022

bors commented Jan 23, 2022

bors commented Jan 23, 2022

rust-timer commented Jan 23, 2022

Make `Decodable` and `Decoder` infallible. #93066

Make `Decodable` and `Decoder` infallible. #93066