Simplify rustc_serialize by dropping support for decoding into JSON

[Mark-Simulacrum]

This PR currently bundles two (somewhat separate) tasks.

First, it removes the JSON Decoder trait impl, which permitted going from JSON to Rust structs. For now, we keep supporting JSON deserialization, but only to Json (an equivalent of serde_json::Value). The primary hard to remove user there is for custom targets -- which need some form of JSON deserialization -- but they already have a custom ad-hoc pass for moving from Json to a Rust struct.

A comment there suggests that it would be impractical to move them to a Decodable-based impl, at least without backwards compatibility concerns. I suspect that if we were widely breaking compat there, it would make sense to use serde_json at this point which would produce better error messages; the types in rustc_target are relatively isolated so we would not particularly suffer from using serde_derive.

The second part of the PR (all but the first commit) is to simplify the Decoder API by removing the non-primitive read_* functions. These primarily add indirection (through a closure), which doesn't directly cause a performance issue (the unique closure types essentially guarantee monomorphization), but does increase the amount of work rustc and LLVM need to do. This could be split out to a separate PR, but is included here in part to help motivate the first part.

Future work might consist of:

• Specializing enum discriminant encoding to avoid leb128 for small enums (since we know the variant count, we can directly use read/write u8 in almost all cases)
• Adding new methods to support faster deserialization (e.g., access to the underlying byte stream)
  • Currently these are somewhat ad-hoc supported by specializations for e.g. Vec<u8>, but other types which could benefit don't today.
• Removing the Decoder trait entirely in favor of a concrete type -- today, we only really have one impl of it modulo wrappers used for specialization-based dispatch.

Highly recommend review with whitespace changes off, as the removal of closures frequently causes things to be de-indented.

[rust-highfive]

r? @jackh726

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

[Mark-Simulacrum]

@bors try @rust-timer queue -- I'm interested to see if the simplification has any effect on compiler bootstrap times or performance.

Follows on groundwork laid by #93681, #93680 to remove JSON usage from a few places. It may merit an MCP, which likely would replicate the PR description fairly closely, but will leave that decision up to the reviewer.

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 5c90ffea61c1b2fb8b732e56afa71dcc1cdeab87 with merge 413eebf76cb8e173694bdcc35c93c55542ec76a0...

[bors]

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

[rust-timer]

Queued 413eebf76cb8e173694bdcc35c93c55542ec76a0 with parent e7aca89, future comparison URL.

[rust-timer]

Finished benchmarking commit (413eebf76cb8e173694bdcc35c93c55542ec76a0): comparison url.

Summary: This benchmark run shows 6 relevant improvements 🎉 but 9 relevant regressions 😿 to instruction counts.

• Average relevant regression: 1.1%
• Average relevant improvement: -2.2%
• Largest improvement in instruction counts: -3.3% on incr-unchanged builds of externs debug
• Largest regression in instruction counts: 1.9% on incr-unchanged builds of clap-rs 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

[Mark-Simulacrum]

I'm pretty sure the small amount of regressions (and improvements, for that matter) in compiler performance is just optimizer noise due to a bunch of different inlining decisions as things get shuffled earlier or later due to simpler Decode impls.

On the other hand, the compiler bootstrap performance improvement is likely not noise, and is a nice ~6 seconds overall. IMO the cleanup here is well-motivated regardless of the comparatively minor regressions in some benchmarks, and primarily incr-unchanged benchmarks at that.

[est31]

Related: #85993

[Mark-Simulacrum]

Yes, this is intentionally intended as a version that does not cause any practical impact (i.e., no features removed) with the goal of enabling it to land without too much discussion that #85993 requires due to dropping support for -Zast-json and the like.

[jackh726]

r? @cjgillot

[nnethercote]

I have a weak preference for preserving read_seq and read_map, because they're providing some useful structure. But either way, this is a lovely change, r=me once you've decided what to do.

[Mark-Simulacrum]

@bors r=nnethercote

read_seq and read_map are both reading just a length from the buffer prior to being called with the current impl, which means that they're tail-calling the passed closure, and I at least find it a little clearer to read the read_usize in plain text (inlined, sort of) than not.

I also think that as we go further along this path (e.g., moving to a Decoder struct) that calculus might shift a little, particularly if some of the users of read_seq start wanting to do something like read_seq_leb128 or so.

[bors]

📌 Commit 5b6932da8c30c5c010e6291b5fc758954a2f3700 has been approved by nnethercote

[ehuss]

@bors ping

[bors]

😪 I'm awake I'm awake

[ehuss]

@bors retry

auto (dist-aarch64-apple, ./x.py dist --stage 2, --build=x86_64-apple-darwin --host=aarch64-apple...
Received request to deprovision: The request was cancelled by the remote provider.

[bors]

⌛ Testing commit c6ad61a with merge 6bdcc5730ff1028182339ecf33e8cf98464128a2...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[Mark-Simulacrum]

@bors retry dist-apple-various didn't start

[bors]

⌛ Testing commit c6ad61a with merge 58a721a...

[bors]

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing 58a721a to master...

[rust-timer]

Finished benchmarking commit (58a721a): comparison url.

Summary: This benchmark run shows 3 relevant improvements 🎉 but 6 relevant regressions 😿 to instruction counts.

• Average relevant regression: 0.8%
• Average relevant improvement: -0.5%
• Largest improvement in instruction counts: -0.5% on incr-unchanged builds of ctfe-stress-4 check
• Largest regression in instruction counts: 1.0% on full builds of externs opt

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