V2 + Deflate serialization support with flate2's stream wrappers #43

marshallpierce · 2017-04-06T14:17:52Z

Performance isn't awful but it isn't great either. Compression is slow and there's not a lot we can do about that.

test deserialize_large_dense_v2          ... bench:  18,970,418 ns/iter (+/- 944,849)
test deserialize_large_dense_v2_deflate  ... bench:  23,991,379 ns/iter (+/- 1,329,386)
test deserialize_large_sparse_v2         ... bench:  17,710,620 ns/iter (+/- 1,145,763)
test deserialize_large_sparse_v2_deflate ... bench:  20,297,392 ns/iter (+/- 1,568,093)

test serialize_large_dense_v2            ... bench:   7,154,361 ns/iter (+/- 307,599)
test serialize_large_dense_v2_deflate    ... bench:  94,926,041 ns/iter (+/- 3,333,186)
test serialize_large_sparse_v2           ... bench:   6,570,606 ns/iter (+/- 639,558)
test serialize_large_sparse_v2_deflate   ... bench:  63,914,342 ns/iter (+/- 4,191,686)

marshallpierce · 2017-04-06T14:20:45Z

benches/serialization.rs

+// least to the user, but for now we'll only take an easily reversible step towards that. There are
+// still several ways the serializer interfaces could change to achieve better performance, so
+// committing to anything right now would be premature.
+trait TestOnlyHypotheticalSerializerInterface {


This, and its cousin in the private test module, are regrettable, but I still have a handful of TODOs to explore in serialization so I don't want to commit to anything just yet. Plus, I suspect it will not be common that users want to polymorphically choose which serializer they use (if for no other reason than that this feature is so new...).

It'd make me happy if we could at least somehow share this trait and its impls between the benchmarks and the tests though.

Yeah, I would like that as well but I couldn't think of a way to do it without making it pub. Thoughts?

I think you may be able to (ab)use the #[path] syntax to include a file with this trait + impls as a module in both...

marshallpierce · 2017-04-06T14:20:56Z

src/serialization/deserializer.rs

+            .ok_or(DeserializeError::UsizeTypeTooSmall)?;
+
+        // TODO reuse deflate buf, or switch to lower-level flate2::Decompress
+        let mut deflate_reader = DeflateDecoder::new(reader.take(payload_len as u64));


allocation :'(

Is it the allocation of the DefaultDecoder you're saddened about? It's a relatively minor allocation, and it's not data-dependent, so I don't know that it'll add significant overhead..?

Yeah, but mostly for intellectual vanity reasons (and a little bit because I want to keep the way to no-heap open) I want serialization and deserialization to be allocation-free. :)

marshallpierce · 2017-04-06T14:22:44Z

src/serialization/v2_deflate_serializer.rs

+        self.compressed_buf.write_u32::<BigEndian>(0)?;
+
+        // TODO pluggable compressors? configurable compression levels?
+        // TODO benchmark https://github.com/sile/libflate


So many TODOs! But none of them are errors, just things I want to explore.

jonhoo · 2017-04-06T15:27:19Z

src/serialization/serialization.rs

@@ -86,6 +90,8 @@
 //!     fn serialize<S: Serializer>(&self, serializer: S) -> Result<(), ()> {
 //!         // not optimal to not re-use the vec and serializer, but it'll work
 //!         let mut vec = Vec::new();
+//!         // pick the format you want to use


I'd like to see this expanded on a little

jonhoo · 2017-04-06T15:29:27Z

src/serialization/serialization.rs

@@ -176,13 +183,19 @@ mod benchmarks;
 mod v2_serializer;
 pub use self::v2_serializer::{V2Serializer, V2SerializeError};

+#[path = "v2_deflate_serializer.rs"]


You know that Rust, when confronted with mod foo, will look for both foo/mod.rs and ./foo.rs by default, right? I don't think this #[path] should be necessary.

You'd think that, and yet without this...

error: cannot declare a new module at this location --> src/serialization/serialization.rs:186:1 | 186 | mod v2_deflate_serializer; | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue #37872 <https://github.com/rust-lang/rust/issues/37872>

Hmm.. Reading issue rust-lang/rust#37872, it seems like this is caused by us using #[path] elsewhere, which sort "taints" the path such that everything needs #[path] annotations. Weird... And unfortunate..

jonhoo · 2017-04-06T15:31:59Z

src/serialization/v2_deflate_serializer.rs

+
+        self.uncompressed_buf.clear();
+        self.compressed_buf.clear();
+        // TODO serialize directly into uncompressed_buf without the buffering inside v2_serializer


Ahh, yeah, that's unfortunate :(

jonhoo · 2017-04-06T15:33:59Z

src/serialization/v2_deflate_serializer.rs

+
+        {
+            // TODO reuse deflate buf, or switch to lower-level flate2::Compress
+            let mut compressor = DeflateEncoder::new(&mut self.compressed_buf, Compression::Default);


I wonder if we could speed up first-time serialization pretty substantially by resizing self.compressed_buf here to some fraction of self.uncompressed_buf.len(). This would avoid doing multiple Vec resizes during the first compression.

Sure, why not. That would make the pessimal case of single-use serializers less bad!

jonhoo · 2017-04-06T17:37:27Z

LGTM!

V2 + Deflate serialization support with flate2's stream wrappers

ba76d87

marshallpierce commented Apr 6, 2017

View reviewed changes

jonhoo reviewed Apr 6, 2017

View reviewed changes

Better comments & other PR feedback

3061676

jonhoo merged commit c071d40 into HdrHistogram:master Apr 6, 2017

marshallpierce deleted the serialize-v2-deflate branch April 11, 2017 23:31

marshallpierce mentioned this pull request Apr 12, 2017

V2 + Deflate [de]serialization #31

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V2 + Deflate serialization support with flate2's stream wrappers #43

V2 + Deflate serialization support with flate2's stream wrappers #43

marshallpierce commented Apr 6, 2017 •

edited

Loading

marshallpierce Apr 6, 2017

jonhoo Apr 6, 2017

marshallpierce Apr 6, 2017

jonhoo Apr 6, 2017

marshallpierce Apr 6, 2017

jonhoo Apr 6, 2017 •

edited

Loading

marshallpierce Apr 6, 2017

marshallpierce Apr 6, 2017

jonhoo Apr 6, 2017

jonhoo Apr 6, 2017

marshallpierce Apr 6, 2017

jonhoo Apr 6, 2017

jonhoo Apr 6, 2017

jonhoo Apr 6, 2017

marshallpierce Apr 6, 2017

jonhoo commented Apr 6, 2017

V2 + Deflate serialization support with flate2's stream wrappers #43

V2 + Deflate serialization support with flate2's stream wrappers #43

Conversation

marshallpierce commented Apr 6, 2017 • edited Loading

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

jonhoo Apr 6, 2017 • edited Loading

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

jonhoo commented Apr 6, 2017

marshallpierce commented Apr 6, 2017 •

edited

Loading

jonhoo Apr 6, 2017 •

edited

Loading