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

Switch JSON serialization from rustc_serialize to serde #418

Closed
1 of 3 tasks
jyn514 opened this issue Mar 16, 2021 · 3 comments
Closed
1 of 3 tasks

Switch JSON serialization from rustc_serialize to serde #418

jyn514 opened this issue Mar 16, 2021 · 3 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@jyn514
Copy link
Member

jyn514 commented Mar 16, 2021

Proposal

Right now, rustc_serialize is rather big:

> tokei compiler/rustc_serialize/ -f
====================================================================================================================
 Language                                                 Files        Lines         Code     Comments       Blanks
====================================================================================================================
 TOML                                                         1           12           10            0            2
--------------------------------------------------------------------------------------------------------------------
 compiler/rustc_serialize/Cargo.toml                                      12           10            0            2
--------------------------------------------------------------------------------------------------------------------
 Rust                                                        10         6263         5449          126          688
 |- Markdown                                                  5          290           82          168           40
 (Total)                                                                6553         5531          294          728
--------------------------------------------------------------------------------------------------------------------
 compiler/rustc_serialize/src/leb128.rs                                  156          129            3           24
 compiler/rustc_serialize/tests/leb128.rs                                 94           71            5           18
 compiler/rustc_serialize/tests/opaque.rs                                276          232            0           44
 compiler/rustc_serialize/src/json/tests.rs                              147          132            1           14
 compiler/rustc_serialize/tests/json.rs                                 1269         1107           10          152
-- compiler/rustc_serialize/src/lib.rs -----------------------------------------------------------------------------
 |- Rust                                                                  32           23            3            6
 |- Markdown                                                               1            0            1            0
 compiler/rustc_serialize/src/lib.rs                                      32           23            3            6
-- compiler/rustc_serialize/src/collection_impls.rs ----------------------------------------------------------------
 |- Rust                                                                 314          287            1           26
 |- Markdown                                                               1            0            1            0
 compiler/rustc_serialize/src/collection_impls.rs                        314          287            1           26
-- compiler/rustc_serialize/src/opaque.rs --------------------------------------------------------------------------
 |- Rust                                                                 714          531           53          130
 |- Markdown                                                               6            0            6            0
 compiler/rustc_serialize/src/opaque.rs                                  714          531           53          130
-- compiler/rustc_serialize/src/serialize.rs -----------------------------------------------------------------------
 |- Rust                                                                 736          635           14           87
 |- Markdown                                                              29            0           24            5
 compiler/rustc_serialize/src/serialize.rs                               736          635           14           87
-- compiler/rustc_serialize/src/json.rs ----------------------------------------------------------------------------
 |- Rust                                                                2525         2302           36          187
 |- Markdown                                                             147            0          124           23
 compiler/rustc_serialize/src/json.rs                                   2525         2302           36          187
====================================================================================================================
 Total                                                       11         6275         5459          126          690
====================================================================================================================

The JSON serialization/deserialization especially is almost exactly the same as the serde API, and there's no need to rewrite it. I propose removing all JSON handling in rustc_serialize.

Why only JSON?

This doesn't propose replacing rustc_serialize anywhere else, because the serde API does have one significant difference: it chooses the (de)serializer per function, not per type:

current rustc_serialize:

trait Decode<D: Decoder> { fn decode ... }

old rustc_serialize (including the deprecated crates.io version):

trait Decode { fn decode<D: Decoder> ... }

serde:

trait Deserialize { fn deserialize<D: Deserializer> ... }

The last two can't have per-decoder behavior, without specialization, which is unsound for e.g. the lifetime parameter of an arena allocator (which the compiler uses extensively). See #329 for more details.

Prior art

Mentors or Reviewers

@eddyb

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

Everywhere I say "rustc_serialize" I mean the in-tree version; I don't know if the crates.io version is maintained or used.

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@jyn514 jyn514 added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Mar 16, 2021
@rustbot
Copy link
Collaborator

rustbot commented Mar 16, 2021

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Mar 16, 2021
@eddyb
Copy link
Member

eddyb commented Mar 16, 2021

#329 is what changed the rustc_serialize API to be sound for specialization. @eddyb tells me the motivation was to allow arena allocating within the deserializer itself.

To be clear: we've always done this for decoding e.g. Ty<'tcx>, there's no way around it (since it's interned in TyCtxt<'tcx>).
What changed is that we were (ab)using unsound specialization before that MCP.

EDIT: I tweaked some of the wording in the original post to make it a bit more clear as to the overall situation.

@eddyb
Copy link
Member

eddyb commented Mar 16, 2021

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Mar 16, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Mar 18, 2021
jyn514 added a commit to jyn514/rust that referenced this issue Mar 28, 2021
Found with https://github.com/est31/warnalyzer.

Dubious changes:
- Is anyone else using rustc_apfloat? I feel weird completely deleting
  x87 support.
- Maybe some of the dead code in rustc_data_structures, in case someone
  wants to use it in the future?
- Don't change rustc_serialize

  I plan to scrap most of the json module in the near future (see
  rust-lang/compiler-team#418) and fixing the
  tests needed more work than I expected.

TODO: check if any of the comments on the deleted code should be kept.
@apiraino apiraino added major-change-accepted A major change proposal that was accepted and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Apr 8, 2021
@rustbot rustbot added the to-announce Announce this issue on triage meeting label Apr 8, 2021
@apiraino apiraino closed this as completed Apr 8, 2021
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Apr 8, 2021
Found with https://github.com/est31/warnalyzer.

Dubious changes:
- Is anyone else using rustc_apfloat? I feel weird completely deleting
  x87 support.
- Maybe some of the dead code in rustc_data_structures, in case someone
  wants to use it in the future?
- Don't change rustc_serialize

  I plan to scrap most of the json module in the near future (see
  rust-lang/compiler-team#418) and fixing the
  tests needed more work than I expected.

TODO: check if any of the comments on the deleted code should be kept.
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 15, 2021
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 3, 2022
Remove all json handling from rustc_serialize

Json is now handled using serde_json. Where appropriate I have replaced json usage with binary serialization (rmeta files) or manual string formatting (emcc linker arg generation).

This allowed for removing and simplifying a lot of code, which hopefully results in faster serialization/deserialization and faster compiles of rustc itself.

Where sensible we now use serde. Metadata and incr cache serialization keeps using a heavily modified (compared to crates.io) rustc-serialize version that in the future could probably be extended with zero-copy deserialization or other perf tricks that serde can't support due to supporting more than one serialization format.

Note that I had to remove `-Zast-json` and `-Zast-json-noexpand` as the relevant AST types don't implement `serde::Serialize`.

Fixes rust-lang#40177

See also rust-lang/compiler-team#418
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this issue Aug 29, 2024
Found with https://github.com/est31/warnalyzer.

Dubious changes:
- Is anyone else using rustc_apfloat? I feel weird completely deleting
  x87 support.
- Maybe some of the dead code in rustc_data_structures, in case someone
  wants to use it in the future?
- Don't change rustc_serialize

  I plan to scrap most of the json module in the near future (see
  rust-lang/compiler-team#418) and fixing the
  tests needed more work than I expected.

TODO: check if any of the comments on the deleted code should be kept.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

4 participants