Remove most specialization use in serialization #73851
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
oli-obk
reviewed
Jun 29, 2020
src/librustc_serialize/serialize.rs
Outdated
| /// * `Encodable` should be used in crates that don't depend on | ||
| /// `librustc_middle`. | ||
| /// * `TyEncodable` should be used for types that are only serialized in crate | ||
| /// metadata or the incremental cache, except for simple enums.where |
There was a problem hiding this comment.
why except for simple enums, and why does this rule not apply to MetadataEncodable? Also there's a trailing where here.
oli-obk
reviewed
Jun 29, 2020
src/librustc_serialize/serialize.rs
Outdated
| /// * `Decodable` should be used in crates that don't depend on | ||
| /// `librustc_middle`. | ||
| /// * `TyDecodable` should be used for types that are only serialized in crate | ||
| /// metadata or the incremental cache, except for simple enums.where |
oli-obk
reviewed
Jun 29, 2020
src/librustc_serialize/serialize.rs
Outdated
| s.emit_usize(*self) | ||
| } | ||
| } | ||
|
|
||
| impl Decodable for usize { | ||
| fn decode<D: Decoder>(d: &mut D) -> Result<usize, D::Error> { | ||
| impl<D: Decoder> Decodable<D> for usize { |
There was a problem hiding this comment.
preexisting, but it seems to me like we should have a macro for all these trivial impls in this file.
oli-obk
reviewed
Jun 29, 2020
| s.emit_str(self) | ||
| } | ||
| } | ||
|
|
||
| impl Encodable for String { | ||
| fn encode<S: Encoder>(&self, s: &mut S) -> Result<(), S::Error> { | ||
| impl<S: Encoder> Encodable<S> for &str { |
There was a problem hiding this comment.
I'm guessing a general impl for &T where T: Encodable<S> will conflict with something?
There was a problem hiding this comment.
It conflicts with the impl for Ty.
oli-obk
approved these changes
Jun 29, 2020
There was a problem hiding this comment.
* If you're adding a new interned type that needs to be en/decodable then the simplest thing way to handle this is:
maybe include this info in the module/crate docs of rustc_serialize
I didn't check the derive code in detail, but since it's working I'm fine merging it.
r=me with the mentioned documentation changes and CI passing
|
This seems like an MCP-worthy change. |
|
Let's see what the perf impact is: @bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit fb818d4321dee29e1938c002c1ff79b0e7eaadff with merge 2cbd21174610f0835db8fb3a01bed54dd04d5d63... |
|
☀️ Try build successful - checks-azure |
|
Queued 2cbd21174610f0835db8fb3a01bed54dd04d5d63 with parent 0ca7f74, future comparison URL. |
|
Finished benchmarking try commit (2cbd21174610f0835db8fb3a01bed54dd04d5d63): comparison url. |
|
Should this be added to rustc-dev-guide in some form? We don't really have any documentation about serialization... |
|
Perf results appear neutral to positive. |
|
I do think we should have this setup documented in the rustc-dev-guide, yes. |
matthewjasper
force-pushed
the
serialize-not-special
branch
from
fb818d4
to
f2adfba
Compare
This was referenced Jul 4, 2020
|
I've opened an MCP and a PR to document serialization in rustc in the guide. |
|
cc #36588 |
eddyb
reviewed
Jul 6, 2020
Comment on lines
+48
to
+53
| ::rustc_serialize::Decoder::read_struct( | ||
| __decoder, | ||
| #ty_name, | ||
| #n_fields, | ||
| |__decoder| { ::std::result::Result::Ok(#construct) }, | ||
| ) |
There was a problem hiding this comment.
It's neat that this works with rustc_serialize, I guess. For #36588 I expected we'd remove all of the features we don't need for the opaque encoding but I don't have anything against being comprehensive.
|
☔ The latest upstream changes (presumably #74330) made this pull request unmergeable. Please resolve the merge conflicts. |
matthewjasper
force-pushed
the
serialize-not-special
branch
2 times, most recently
from
ddf030a
to
560918b
Compare
|
@bors p=1 rollup=iffy |
|
☔ The latest upstream changes (presumably #74662) made this pull request unmergeable. Please resolve the merge conflicts. |
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
@bors r=oli-obk |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit c4f91bb has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
This was referenced Aug 15, 2020
|
Final perf results show a very slight regression in instruction counts unlike the earlier perf run, which showed a slight improvement. Is this actionable @matthewjasper? |
|
I don't think there's anything obvious to do for this. Maybe profiling would show something. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Switching from specialization to min_specialization in the compiler made the unsoundness of how we used these traits pretty clear. This changes how the
EncodableandDecodabletraits work to be more friendly for types need aTyCtxtto deserialize.The alternative design of having both
EncodableandTyEncodabletraits was considered, but doesn't really work because the following impls would conflict:How-to guide
Rustc(De|En)codableis now spelledTy(De|En)coableinrustc_middle,Metadata(En|De)codableinrustc_metadatawhere needed, and(De|En)codableeverywhere else.(De|En)codableshouldn't be much different.ty::Predicatedoes, and not what all of the other interned types do)Ty(En|De)codableon the inner typeEncodable<impl TyEncoder>by forwarding to the inner type.Decodable<impl TyDecoder>by decoding the inner type and then creating the wrapper around that (using thetcxfrom the decoder as needed).cc @rust-lang/compiler for opinions on this change
r? @oli-obk