Move from protobuf to bincode for a wire format #85

alexcrichton · 2017-03-22T17:32:55Z

This commit migrates away from the protobuf crate to instead just working with
bincode on the wire as a serialization format. This is done by leveraging a few
different crates:

The bincode and serde_derive crates are used to define serialization
for Rust structures as well as provide a bincode implementation.
The tokio_io::codec::length_delimited module implements framing via length
prefixes to transform an asynchronous stream of bytes into a literal Stream
of BytesMut.
The tokio_serde_bincode crate is then used to tie it all together, parsing
these BytesMut as the request/response types of sccache.

Most of the changes here are related to moving away from the protobuf API
throughout the codebase (e.g. has_foo and take_foo) towards a more
rustic-ish API that just uses enums/structs. Overall it felt quite natural (as
one would expect) to just use the raw enum/struct values.

This may not be quite as performant as before but that doesn't really apply to
sccache's use case where perf is hugely dominated by actually compiling and
hashing, so I'm not too too worried about that.

My personal motivation for this is twofold:

Using protobuf was a little clunky throughout the codebase and definitely
had some sharp edges that felt good to smooth out.
There's currently what I believe some mysterious segfault and/or stray write
happening in sccache and I'm not sure where. The protobuf crate had a lot
of unsafe code and in lieu of actually auditing it I figured it'd be good
to kill two birds with one stone. I have no idea if this fixes my segfault
problem (I never could reproduce it) but I figured it's worth a shot.

luser · 2017-03-23T10:42:44Z

Excellent! I've wanted to drop protobuf for serde for a while now (#20). I'm not sure that the specific wire format matters all that much in terms of perf, and being able to packet-sniff the traffic and read the JSON could prove useful.

The only thing I worried about with using JSON was the compile response where a large part of the body is Vec<u8>. Presumably that will blow up to enormous size in JSON? It still may not matter in the grand scheme of things.

alexcrichton · 2017-03-23T14:16:01Z

Oh jeez that's a very good point. It is true that swapping a different serde protocol should be pretty easy and another possible option is bincode which I believe Servo uses (but would be harder to inspect on the wire).

I'll do some testing locally to see what happens.

alexcrichton · 2017-03-23T14:28:25Z

Yeah looks like b"foo" is serialized as [100,111,111], from 3 to 13 bytes. I'll switch to bincode!

alexcrichton · 2017-03-23T15:05:03Z

Ok switched over to bincode!

luser · 2017-03-23T15:59:39Z

Wire inspection is not a deal breaker (given that we control both ends of the connection and already have logging), it was just a possible nice-to-have. :)

This is an abstraction of the Tokio stack now and doesn't have much impact on sccache itself but I figured it'd be good to update a few deps and pick up the recent version of things! Plus that and I'd like to start using the length_delimited module soon...

luser

I think you unintentionally removed the compile locally on EOF behavior you recently added, but aside from that this looks great! The code is so much more readable than it was with protobuf.

Feel free to merge after you fix that, I noted a couple of other nits you might want to fix as well.

luser · 2017-03-27T10:46:01Z

src/client.rs

@@ -52,28 +45,29 @@ impl ServerConnection {
    }

    /// Send `request` to the server, read and return a `ServerResponse`.


nit: the doc comment still mentions ServerResponse

luser · 2017-03-27T10:47:12Z

src/client.rs

+        BigEndian::write_u32(&mut len, bytes.len() as u32);
+        self.writer.write_all(&len)?;
+        self.writer.write_all(&bytes)?;
+        self.writer.flush()?;
        trace!("ServerConnection::request: sent request");
        self.read_one_response()
    }

    /// Read a single `ServerResponse` from the server.


same nit as above

luser · 2017-03-27T10:48:24Z

src/client.rs

-            is.read_raw_varint32()
-        });
+        let mut bytes = [0; 4];
+        self.reader.read_exact(&mut bytes)?;


Would things look nicer overall if we used tokio here instead of blocking IO? Originally I used blocking IO in the client because the mio stuff was so complicated.

Nah I think they'd basically be equivalent. As soon as we'd want to do anything else, though, such as timeouts, then we'd definitely want to use Tokio as it'd look quite a bit better.

The tokio version would basically just do the futures equivalent of this, using tokio_io combinators, and a little less ergonomically.

luser · 2017-03-27T10:50:31Z

src/commands.rs

-    trace!("request_compile: {:?}", compile);
-    req.set_compile(compile);
+    let req = Request::Compile(Compile {
+        exe: exe.as_ref().to_str().ok_or("bad exe")?.to_owned(),


as a follow-up, we could now just use OsStr or OsString all the way for the values from the client commandline.

Yeah I tried to do that but Serialize/Deserialize wasn't implemented for one of them :(

Actually... I'll send a PR!

Ok this is now tracked at serde-rs/serde#823

luser · 2017-03-27T10:51:15Z

src/commands.rs

-    }
-    if response.has_retcode() {
-        let ret = response.get_retcode();
+    try!(stdout.write_all(&response.stdout));


Switch these to ? while you're here?

luser · 2017-03-27T10:52:58Z

src/commands.rs

-                    }
-                }
-
-                // Currently the shutdown behavior of the remote sccache server


I don't see that you replaced this behavior anywhere (compile locally on EOF from server). Presumably you do want to keep this in some form?

Oops, updated!

luser · 2017-03-27T10:54:33Z

src/server.rs

+                    match status.code() {
+                        Some(code) => res.retcode = Some(code),
+                        None => res.signal = Some(get_signal(status)),
+                    };
                    //TODO: sort out getting signal return on Unix


You can remove this TODO that you fixed. :)

luser · 2017-03-27T10:56:16Z

src/server.rs

@@ -718,24 +704,25 @@ impl ServerStats {
    fn to_cache_statistics(&self) -> Vec<CacheStatistic> {


We could probably do better here now just by defining the CacheStats type to use the CacheStat type for its fields directly, but we can sort that out in a followup.

luser · 2017-03-27T10:56:59Z

src/server.rs

+                };
+                $vec.push(CacheStatistic {
+                    name: String::from($name),
+                    value: CacheStat::String(s),


I probably should have added a CacheStat::Duration for clarity, eh?

This commit migrates away from the `protobuf` crate to instead just working with bincode on the wire as a serialization format. This is done by leveraging a few different crates: * The `bincode` and `serde_derive` crates are used to define serialization for Rust structures as well as provide a bincode implementation. * The `tokio_io::codec::length_delimited` module implements framing via length prefixes to transform an asynchronous stream of bytes into a literal `Stream` of `BytesMut`. * The `tokio_serde_bincode` crate is then used to tie it all together, parsing these `BytesMut` as the request/response types of sccache. Most of the changes here are related to moving away from the protobuf API throughout the codebase (e.g. `has_foo` and `take_foo`) towards a more rustic-ish API that just uses enums/structs. Overall it felt quite natural (as one would expect) to just use the raw enum/struct values. This may not be quite as performant as before but that doesn't really apply to sccache's use case where perf is hugely dominated by actually compiling and hashing, so I'm not too too worried about that. My personal motivation for this is twofold: 1. Using `protobuf` was a little clunky throughout the codebase and definitely had some sharp edges that felt good to smooth out. 2. There's currently what I believe some mysterious segfault and/or stray write happening in sccache and I'm not sure where. The `protobuf` crate had a lot of `unsafe` code and in lieu of actually auditing it I figured it'd be good to kill two birds with one stone. I have no idea if this fixes my segfault problem (I never could reproduce it) but I figured it's worth a shot.

alexcrichton force-pushed the less-protobuf branch 2 times, most recently from daff2b4 to 51bd77b Compare March 23, 2017 02:10

alexcrichton force-pushed the less-protobuf branch from 51bd77b to f536446 Compare March 23, 2017 15:04

alexcrichton changed the title ~~Move from protobuf to JSON for a wire format~~ Move from protobuf to bincode for a wire format Mar 23, 2017

alexcrichton force-pushed the less-protobuf branch 2 times, most recently from 75196f9 to 07c64aa Compare March 25, 2017 01:49

luser approved these changes Mar 27, 2017

View reviewed changes

alexcrichton force-pushed the less-protobuf branch from 07c64aa to 6b72a1c Compare March 27, 2017 16:24

alexcrichton force-pushed the less-protobuf branch from 6b72a1c to a996ae6 Compare March 27, 2017 16:29

alexcrichton merged commit a0b7cc1 into mozilla:master Mar 27, 2017

alexcrichton deleted the less-protobuf branch March 27, 2017 17:14

dtolnay mentioned this pull request Mar 28, 2017

Use serde's OsStr and OsString impls #93

Closed

luser mentioned this pull request Apr 7, 2017

Look into replacing protobuf with serde #20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move from protobuf to bincode for a wire format #85

Move from protobuf to bincode for a wire format #85

alexcrichton commented Mar 22, 2017 •

edited

Loading

luser commented Mar 23, 2017

alexcrichton commented Mar 23, 2017

alexcrichton commented Mar 23, 2017

alexcrichton commented Mar 23, 2017

luser commented Mar 23, 2017

luser left a comment

luser Mar 27, 2017

luser Mar 27, 2017

luser Mar 27, 2017

alexcrichton Mar 27, 2017

luser Mar 27, 2017

alexcrichton Mar 27, 2017

alexcrichton Mar 27, 2017

alexcrichton Mar 27, 2017

luser Mar 27, 2017

luser Mar 27, 2017

alexcrichton Mar 27, 2017

luser Mar 27, 2017

luser Mar 27, 2017

luser Mar 27, 2017

		@@ -52,28 +45,29 @@ impl ServerConnection {
		}

		/// Send `request` to the server, read and return a `ServerResponse`.

		@@ -718,24 +704,25 @@ impl ServerStats {
		fn to_cache_statistics(&self) -> Vec<CacheStatistic> {

Move from protobuf to bincode for a wire format #85

Move from protobuf to bincode for a wire format #85

Conversation

alexcrichton commented Mar 22, 2017 • edited Loading

luser commented Mar 23, 2017

alexcrichton commented Mar 23, 2017

alexcrichton commented Mar 23, 2017

alexcrichton commented Mar 23, 2017

luser commented Mar 23, 2017

luser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Mar 22, 2017 •

edited

Loading