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

Smarter deserialize_bytes and deserialize_byte_buf #115

Merged
merged 2 commits into from Feb 23, 2017

Conversation

@tikue
Copy link
Contributor

tikue commented Feb 22, 2017

It seems we can just read all at once rather than reading byte by byte.

@TyOverby
Copy link
Collaborator

TyOverby commented Feb 23, 2017

This looks really good, thanks! While you are in the read_vec code, could you make a little change?

let mut buffer = Vec::new();
try!(self.reader.by_ref().take(len as u64).read_to_end(&mut buffer));

could be replaced with

let mut bytes = Vec::with_capacity(len);
unsafe { bytes.set_len(len); }
try!(self.reader.read_exact(&mut bytes[..]));

and you'd probably see even better performance.

(this implementation was ruthlessly stolen from #116)

@@ -192,13 +195,13 @@ impl<'a, R: Read> serde::Deserializer for &'a mut Deserializer<R> {
fn deserialize_bytes<V>(self, visitor: V) -> Result<V::Value>
where V: serde::de::Visitor,
{
self.deserialize_seq(visitor)
visitor.visit_byte_buf(try!(self.read_vec()))

This comment has been minimized.

@TyOverby

TyOverby Feb 23, 2017

Collaborator

shouldn't this be a call to visit_bytes?

This comment has been minimized.

@tikue

tikue Feb 23, 2017

Author Contributor

Done.

@tikue
Copy link
Contributor Author

tikue commented Feb 23, 2017

Great suggestion, thanks -- done!

@tikue tikue force-pushed the tikue:faster-byte-buf-deserialization branch 3 times, most recently from 6931c26 to 2984b85 Feb 23, 2017
Also, call visit_bytes in deserialize_bytes rather than visit_byte_buf.
@tikue tikue force-pushed the tikue:faster-byte-buf-deserialization branch from 2984b85 to 676d938 Feb 23, 2017
@dtolnay
Copy link
Collaborator

dtolnay commented Feb 23, 2017

Still not as smart as it could be. I filed #118 to follow up in another PR.

@TyOverby
Copy link
Collaborator

TyOverby commented Feb 23, 2017

@dtolnay: sounds good! I'll merge this and start in on the recommendations in #118

@TyOverby TyOverby merged commit 324dcb2 into servo:master Feb 23, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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