RUST-871 Serialize directly to BSON bytes in insert operations #406

patrickfreed · 2021-07-29T22:20:21Z

This PR leverages the bson::to_vec function that was introduced in mongodb/bson-rust#279 to greatly increase the performance of inserts. It also introduces the framework for updating other operations to skip the Document step during serialization, though that is left as future work for once the raw document API is introduced.

patrickfreed · 2021-07-29T22:25:21Z

benchmarks/src/main.rs

@@ -444,7 +444,7 @@ fn parse_ids(matches: ArgMatches) -> Vec<bool> {
    ids
 }

-#[cfg_attr(feature = "tokio-runtime", tokio::main)]
+#[cfg_attr(feature = "tokio-runtime", tokio::main(flavor = "current_thread"))]


Since the benchmarks are single threaded, this ends up giving a nice boost by avoiding the tokio overhead of managing a thread pool.

patrickfreed · 2021-07-29T22:25:38Z

src/bson_util/mod.rs

@@ -164,128 +169,30 @@ where
        .ok_or_else(|| D::Error::custom(format!("could not deserialize u64 from {:?}", bson)))
 }

-pub fn doc_size_bytes(doc: &Document) -> u64 {


These were no longer needed as we now just use the length of the serialized vec.

patrickfreed · 2021-07-29T22:26:08Z

src/bson_util/mod.rs

-            Some(val) => val,
-            None => break,
-        };
+fn num_decimal_digits(mut n: usize) -> u64 {


the old implementation of this method was incorrect actually so I rewrote it.

patrickfreed · 2021-07-29T22:27:03Z

src/bson_util/mod.rs

+/// If the BSON is not properly formatted, an internal error would be returned.
+///
+/// TODO: RUST-924 replace this with raw document API usage.
+pub(crate) fn raw_get(doc: &[u8], key: &str) -> Result<Option<Bson>> {


Unfortunately this function needs to live in the driver for now as we need to be able to comb through the serialized command for _id values, but it can go away shortly once RUST-924 is complete.

patrickfreed · 2021-07-29T22:27:47Z

src/bson_util/mod.rs

+        Error::from(ErrorKind::Internal {
+            message: "failed to read from serialized document".to_string(),
+        })
+    }


The documents queried here should always be valid as they're coming straight out of our serializer, so any error encountered here is mapped to an internal error.

patrickfreed · 2021-08-02T20:04:00Z

src/operation/insert/mod.rs


 #[derive(Debug)]
-pub(crate) struct Insert<T> {
+pub(crate) struct Insert<'a, T> {


Previously this T was always actually a reference. This now makes it required to be a reference, which allows us to clone the input.

patrickfreed · 2021-08-02T20:04:46Z

src/operation/insert/mod.rs

-
-            let doc_size = bson_util::array_entry_size_bytes(i, &doc);
+            let mut doc = bson::to_vec(d)?;
+            let id = match bson_util::raw_get(doc.as_slice(), "_id")? {


Since we can't inspect the T for an _id without requiring it implement a trait, we scan through the raw document for it instead.

patrickfreed · 2021-08-02T20:05:49Z

src/operation/insert/mod.rs

+                Some(b) => b,
+                None => {
+                    let oid = ObjectId::new();
+


If it's not in there, we write the new element into the beginning of the document. Once we have the raw document API we could update this to use it.

Can we add a TODO comment for this?

patrickfreed · 2021-08-02T20:07:57Z

src/operation/insert/mod.rs

+        Ok(Command::new("insert".to_string(), self.ns.db.clone(), body))
+    }
+
+    fn serialize_command(&mut self, cmd: Command<Self::Command>) -> Result<Vec<u8>> {


Since we already have raw BSON for the documents, we can't just use serde to serialize it, as it'll attempt to do so as a a Binary. Instead, we serialize the rest of the command and manually append the serialized documents. Again, this will be easier with the raw document API.

src/client/auth/mod.rs

src/cmap/conn/command.rs

src/cmap/conn/mod.rs

isabelatkinson · 2021-08-04T21:34:57Z

src/operation/abort_transaction/mod.rs

    type Response = CommandResponse<WriteConcernOnlyBody>;

    const NAME: &'static str = "abortTransaction";

-    fn build(&mut self, _description: &StreamDescription) -> Result<Command> {
+    fn build(&mut self, _description: &StreamDescription) -> Result<Command<Self::Command>> {


I don't think the addition of Self::Command here is necessary because the default T for Command is already Document -- can we revert this change or change the rest of the operations whose build signature wasn't updated (e.g. ListCollections) for consistency?

isabelatkinson · 2021-08-04T21:36:55Z

src/operation/insert/mod.rs

+                Some(b) => b,
+                None => {
+                    let oid = ObjectId::new();
+


Can we add a TODO comment for this?

isabelatkinson · 2021-08-04T21:44:49Z

src/test/spec/unified_runner/test_file.rs

    pub server_api: Option<ServerApi>,
 }

 fn default_uri() -> String {
    DEFAULT_URI.clone()
 }

+pub fn deserialize_server_api<'de, D>(


Why was this custom implementation needed?

The server expects the fields to be named apiVersion, apiStrict, and apiDeprecationErrors, but the test format drops all of the api- prefixes. Now that we're using the Serialize implementation for ServerApi in the driver, we needed to introduce a custom one for deserializing from the test format.

I know this is only used within this test file, but for clarity can we rename this serialization function? Maybe deserialize_server_api_for_testing or deserialize_server_api_test_format?

Ah in that case I think you should be able to change the rename attributes on the fields in ServerApi to #[serde(rename(serialize = "apiVersion"))] so that the renames only apply to serialization, and then add #[serde(rename_all = "camelCase", deny_unknown_fields)] to the top of the struct for deserialization.

Oh cool, I didn't know that was possible. Unfortunately, it looks like we'll still need the separate implementation though, since deny_unknown_fields can't be used with flatten (see https://serde.rs/attr-flatten.html). flatten is needed with server API since they're all top level fields of commands rather than in a subdocument.

I did update the fuinction to be more precisely named (went with the test_format suggestion)

NBSquare

LGTM! Only a few small things from me.

NBSquare · 2021-08-05T13:47:49Z

src/client/executor.rs

+
+        let serialized = op.serialize_command(cmd)?;
+        let raw_cmd = RawCommand {
+            name: cmd_name.clone(),


As far as I can tell this is the only place cmd_name is being used, so this seems like a spurious clone to me.

I thought so too, but it's actually also used below when emitting the command succeeded / failed event.

NBSquare · 2021-08-05T14:18:49Z

src/test/spec/unified_runner/test_file.rs

    pub server_api: Option<ServerApi>,
 }

 fn default_uri() -> String {
    DEFAULT_URI.clone()
 }

+pub fn deserialize_server_api<'de, D>(


I know this is only used within this test file, but for clarity can we rename this serialization function? Maybe deserialize_server_api_for_testing or deserialize_server_api_test_format?

patrickfreed commented Jul 29, 2021

View reviewed changes

patrickfreed added 4 commits August 2, 2021 16:19

serialize insert to raw bson

3d3ed57

fix lint

b2afcb1

various cleanup

845a5d0

support setting recovery token in Command

e2db5d7

patrickfreed force-pushed the RUST-871/raw-serialize branch from f9597f7 to e2db5d7 Compare August 2, 2021 20:20

patrickfreed commented Aug 2, 2021

View reviewed changes

fix Message::with_command, rename deprecation_errors

d7dab28

patrickfreed marked this pull request as ready for review August 2, 2021 21:17

patrickfreed requested review from isabelatkinson, abr-egn and NBSquare August 2, 2021 21:17

abr-egn reviewed Aug 3, 2021

View reviewed changes

src/client/auth/mod.rs Show resolved Hide resolved

src/cmap/conn/command.rs Show resolved Hide resolved

src/cmap/conn/mod.rs Outdated Show resolved Hide resolved

fix typo

fd13cec

abr-egn approved these changes Aug 3, 2021

View reviewed changes

stop returning result from command setters

fd8a2f0

isabelatkinson reviewed Aug 4, 2021

View reviewed changes

patrickfreed added 2 commits August 4, 2021 18:30

use Command instead of Command<Self::Command> where possible

a3c17c8

add TODO

8a1a93a

NBSquare reviewed Aug 5, 2021

View reviewed changes

clearer name of deserialize function

15db10d

isabelatkinson approved these changes Aug 5, 2021

View reviewed changes

patrickfreed merged commit 7f03ec5 into mongodb:master Aug 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-871 Serialize directly to BSON bytes in insert operations #406

RUST-871 Serialize directly to BSON bytes in insert operations #406

patrickfreed commented Jul 29, 2021

patrickfreed Jul 29, 2021

patrickfreed Jul 29, 2021

patrickfreed Jul 29, 2021

patrickfreed Jul 29, 2021

patrickfreed Jul 29, 2021

patrickfreed Aug 2, 2021

patrickfreed Aug 2, 2021

patrickfreed Aug 2, 2021

isabelatkinson Aug 4, 2021

patrickfreed Aug 4, 2021

patrickfreed Aug 2, 2021

isabelatkinson Aug 4, 2021

patrickfreed Aug 4, 2021

isabelatkinson Aug 4, 2021

isabelatkinson Aug 4, 2021

patrickfreed Aug 4, 2021

NBSquare Aug 5, 2021

isabelatkinson Aug 5, 2021

patrickfreed Aug 5, 2021

NBSquare left a comment

NBSquare Aug 5, 2021

patrickfreed Aug 5, 2021

NBSquare Aug 5, 2021

RUST-871 Serialize directly to BSON bytes in insert operations #406

RUST-871 Serialize directly to BSON bytes in insert operations #406

Conversation

patrickfreed commented Jul 29, 2021

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

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

NBSquare 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