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

Specialize for deserializing from &[u8] #119

Closed
dtolnay opened this issue Feb 23, 2017 · 11 comments
Closed

Specialize for deserializing from &[u8] #119

dtolnay opened this issue Feb 23, 2017 · 11 comments
Milestone

Comments

@dtolnay
Copy link
Collaborator

@dtolnay dtolnay commented Feb 23, 2017

Like in the JSON deserializer, Bincode should have a specialized codepath for deserializing from &[u8]. This is an enormous improvement because it would no longer require copying strings into a temp buffer.

@TyOverby
Copy link
Collaborator

@TyOverby TyOverby commented Feb 23, 2017

When you say "specialized", do you mean, like, the Rust language feature "specialization"?

If so, I'm not seeing how that is used in the json deserializer.

@dtolnay
Copy link
Collaborator Author

@dtolnay dtolnay commented Feb 23, 2017

Eventually once specialization is stable, yes. For now it is done with this trait by having the caller select the right entry point based on the type of data that they have - from_slice vs from_str vs from_reader vs from_iter.

@TyOverby
Copy link
Collaborator

@TyOverby TyOverby commented Feb 23, 2017

I'm not a huge fan of adding a non-standard Read trait for this :/

I also don't think that Bincode will benefit from this as much as Json would. Because everything that comes out of bincode must be owned, practically every structure that gets deserialized will be copied.

@TyOverby
Copy link
Collaborator

@TyOverby TyOverby commented Feb 23, 2017

I've used Bincode a lot for network communication and save-games and I've never written any code that would hit this optimization. In fact, I think you'd need to custom-implement the Serde Deserialize trait in order to get any benefit.

@dtolnay
Copy link
Collaborator Author

@dtolnay dtolnay commented Feb 23, 2017

Ah yeah, deserialize_str is the overwhelmingly common case for JSON but for Bincode it is deserialize_string. Good call.

@dtolnay dtolnay closed this Feb 23, 2017
@TyOverby TyOverby reopened this Feb 23, 2017
@TyOverby TyOverby added this to the unknown milestone Feb 23, 2017
@TyOverby
Copy link
Collaborator

@TyOverby TyOverby commented Feb 23, 2017

re-opening and adding to milestone: unknown. Some people doing custom deserialize implementations could benefit from this. But I think we should wait until stabilization stabilizes.

@dtolnay
Copy link
Collaborator Author

@dtolnay dtolnay commented Feb 23, 2017

What are the objections against using a trait for this in lieu of specialization?

@TyOverby
Copy link
Collaborator

@TyOverby TyOverby commented Feb 23, 2017

As I understand it, the trait would have to be exposed to the user right? I'd rather have Bincode use traits and types just from std and serde.

@dtolnay
Copy link
Collaborator Author

@dtolnay dtolnay commented Feb 23, 2017

Yes but the vast majority of users never see it. Most people just use this function where they pass in a &[u8] and they get back a T.

pub fn from_slice<T>(v: &[u8]) -> Result<T>
    where T: de::Deserialize,
{
    from_trait(read::SliceRead::new(v))
}

Or this function where they pass in an io::Read and get back a T.

pub fn from_reader<R, T>(rdr: R) -> Result<T>
    where R: io::Read,
          T: de::Deserialize,
{
    from_iter(rdr.bytes())
}

Really the only time a user would see this is in the rare case that someone stores a long-lived deserializer (typically a stream deserializer) in a struct, but that is almost nobody:

struct MyStruct<R: serde_json::de::Read> {
    the_goods: serde_json::StreamDeserializer<R, serde_json::Value>,
}
@ZoeyR
Copy link
Collaborator

@ZoeyR ZoeyR commented Jul 24, 2017

Isn't this now resolved since we have the BincodeRead trait and the SliceReader impl?

@TyOverby
Copy link
Collaborator

@TyOverby TyOverby commented Oct 14, 2017

@dgriffen: yeah, this should be closed

@TyOverby TyOverby closed this Oct 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.