RUST-536 Eliminate redundant clones #377

patrickfreed · 2021-06-23T17:44:07Z

This PR removes the need to clone input documents or command responses in the driver, resulting in significant performance improvements (from 15-40% depending on the benchmark). It also removes unneeded clones that were used in the benchmark runner now that references can be accepted in the insert family of methods (#331). The benchmark runner was also updated to compile, as it had slipped out of date with the breaking changes in the driver / tokio.

patrickfreed · 2021-06-23T17:45:54Z

src/bson_util/mod.rs

@@ -171,7 +166,7 @@ where
        .ok_or_else(|| D::Error::custom(format!("could not deserialize u64 from {:?}", bson)))
 }

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


this was changed to always use a 64-bit integer in case we're on a 32-bit system and a large document is encountered.

patrickfreed · 2021-06-23T17:52:27Z

src/coll/mod.rs

-/// Maximum size in bytes of an insert batch.
-/// This is intentionally less than the actual max document size, which is 16*1024*1024 bytes, to
-/// allow for overhead in the command document.
-const MAX_INSERT_DOCS_BYTES: usize = 16 * 1000 * 1000;


we previously had a hard-coded value for this, but we're actually meant to derive this from the hello response. Also, we baked in a healthy ~5% overhead allotment here, but there is already 16 KiB of overhead allowed on top of the value returned from hello, so imposing our own wiggle room isn't necessary.

patrickfreed · 2021-06-23T17:54:24Z

src/coll/mod.rs

        &self,
-        docs: impl IntoIterator<Item = impl Borrow<T>>,
+        docs: impl IntoIterator<Item = D>,


We need an explicit generic type here in order to collect the things being borrowed into a Vec, which is needed to ensure that the references live long enough.

Another option would be let ds: Vec<_> = ..., I think?

Ah neat, that does work! I wasn't aware this was possible with opaque types. Updated to use that.

patrickfreed · 2021-06-23T17:56:20Z

src/coll/mod.rs

-            let current_batch = remaining_docs;
+        while n_attempted < ds.len() {
+            let docs: Vec<&T> = ds.iter().skip(n_attempted).map(Borrow::borrow).collect();
+            let insert = Insert::new(self.namespace(), docs, options.clone());


In order to eliminate clones, we pass the T references into Insert instead of Documents. However, this means that at this level we can't know how many documents should be included in the batch, so we pass all of the references each time (skipping the ones completed) and need to discover the batch size from the return values of execute_operation.

I think you can get the same elimination of clones without repeating serialization if you store a Vec<Document> in this method and then pass a Vec<&Document> to Insert::new.

The problem persists with that I think, since we wouldn't be able to move the Vec<&Document> into the command document. By delaying serialization until building the document, we can move the documents directly and just hope we don't have to serialize again in a retry attempt.

patrickfreed · 2021-06-23T17:57:14Z

src/coll/mod.rs

-                                .inserted_ids
-                                .insert(index + n_attempted - current_batch_size, id);
-                        }
+                    let current_batch_size = result.inserted_ids.len();


If it succeeded, we know the batch size is just the number of documents that were inserted.

patrickfreed · 2021-06-23T17:57:58Z

src/coll/mod.rs

+                        // for ordered inserts this size will be incorrect, but knowing the batch
+                        // size isn't needed for ordered failures since we
+                        // return immediately from them anyways.
+                        let current_batch_size = bw.inserted_ids.len()


If it failed (and the insert was not ordered), then we know the batch size is the number of inserted documents + the number of ones that failed. If it's ordered we can't figure it out from that alone, but that's okay because we're going to return early anyways and don't need to know the batch size.

patrickfreed · 2021-06-23T18:04:21Z

src/error.rs

@@ -510,13 +508,16 @@ pub struct BulkWriteFailure {

    /// The error that occurred on account of write concern failure.
    pub write_concern_error: Option<WriteConcernError>,
+
+    pub(crate) inserted_ids: HashMap<usize, Bson>,


This is necessary to track how many writes were attempted in the event of an error in an unordered bulk write, but it's also something that users might find useful (RUST-260).

patrickfreed · 2021-06-23T18:05:39Z

src/operation/mod.rs

@@ -66,7 +66,8 @@ pub(crate) trait Operation {
    const NAME: &'static str;

    /// Returns the command that should be sent to the server as part of this operation.
-    fn build(&self, description: &StreamDescription) -> Result<Command>;
+    /// The operation may store some additional state that is required for handling the response.
+    fn build(&mut self, description: &StreamDescription) -> Result<Command>;


This was updated to take &mut self so that Insert could store the _id's it appended to the documents it inserts.

It's a pity that so many sites had to be changed to accommodate the single Insert case; I wonder if it would be better to keep this as &self and cheat with a RefCell in Index?

Yeah, it is a bit unfortunate. My original implementation actually avoided it by introducing a build_mut method in Operation with a default implementation that called into build, but that left Insert in a weird place where it implemented build_mut but not build, so I just decided to bite the bullet and do a search and replace so that all operations were consistent (it actually ended up being a pretty quick refactor despite all the changes).

Regarding tricks with RefCell or Mutex and what not, I think it's best we not hide any of the mutability either, since it's now important that build gets called before handle_response, and build being a mutating method helps signify that to some extent.

patrickfreed · 2021-06-23T18:08:15Z

src/operation/insert/mod.rs

+            .take(description.max_write_batch_size as usize)
+            .enumerate()
+        {
+            let mut doc = bson::to_document(d)?;


By delaying the serialization until build, we can avoid cloning here. We will have to repeat the serialization in the retry making that case slower, but the common case gets much faster.

patrickfreed · 2021-06-23T18:08:55Z

src/operation/insert/mod.rs

+            let doc_size = bson_util::array_entry_size_bytes(i, &doc);
+
+            if (size + doc_size) <= description.max_bson_object_size as u64 {
+                if self.inserted_ids.len() <= i {


only populated the inserted_ids vector if this is the first time build is being called.

abr-egn · 2021-06-24T16:33:56Z

src/coll/mod.rs

        &self,
-        docs: impl IntoIterator<Item = impl Borrow<T>>,
+        docs: impl IntoIterator<Item = D>,


Another option would be let ds: Vec<_> = ..., I think?

abr-egn · 2021-06-24T16:55:10Z

src/coll/mod.rs

-            let current_batch = remaining_docs;
+        while n_attempted < ds.len() {
+            let docs: Vec<&T> = ds.iter().skip(n_attempted).map(Borrow::borrow).collect();
+            let insert = Insert::new(self.namespace(), docs, options.clone());


I think you can get the same elimination of clones without repeating serialization if you store a Vec<Document> in this method and then pass a Vec<&Document> to Insert::new.

abr-egn · 2021-06-24T17:01:26Z

src/error.rs

 }

 impl BulkWriteFailure {
    pub(crate) fn new() -> Self {
        BulkWriteFailure {
            write_errors: None,
            write_concern_error: None,
+            inserted_ids: Default::default(),


Out of curiosity, why not HashMap::new() ?

The choice was arbitrary. I think maybe I wasn't sure if HashMap had been imported or not so I went with Default.

abr-egn · 2021-06-24T17:26:25Z

src/operation/mod.rs

@@ -66,7 +66,8 @@ pub(crate) trait Operation {
    const NAME: &'static str;

    /// Returns the command that should be sent to the server as part of this operation.
-    fn build(&self, description: &StreamDescription) -> Result<Command>;
+    /// The operation may store some additional state that is required for handling the response.
+    fn build(&mut self, description: &StreamDescription) -> Result<Command>;


It's a pity that so many sites had to be changed to accommodate the single Insert case; I wonder if it would be better to keep this as &self and cheat with a RefCell in Index?

patrickfreed added 8 commits June 23, 2021 13:12

fix benchmark compilation

62cc0c4

borrow in insertion

1943d04

print median iteration time

238f455

take ownership of self in CommandResponse::body

9ce674e

late serialization

9146bdd

borrow in insert_one benchmark

66e7a3a

rustfmt

7d1c9f5

remove redundant limit

b0ec6eb

patrickfreed commented Jun 23, 2021

View reviewed changes

patrickfreed added 4 commits June 23, 2021 14:10

add docstring to array_entry_size_bytes

abea083

fix typo

7809c10

fix insert operation unit tests

c06b7f0

fix sdam spec tests

10ecdd7

patrickfreed marked this pull request as ready for review June 23, 2021 19:49

patrickfreed requested review from isabelatkinson and abr-egn June 23, 2021 19:50

abr-egn reviewed Jun 24, 2021

View reviewed changes

drop generic parameter

49ed35d

abr-egn approved these changes Jun 24, 2021

View reviewed changes

isabelatkinson approved these changes Jun 25, 2021

View reviewed changes

patrickfreed merged commit 3312975 into mongodb:master Jun 25, 2021

patrickfreed mentioned this pull request Oct 11, 2022

RUST-1488 Populate inserted_ids when an insert_many fails #761

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-536 Eliminate redundant clones #377

RUST-536 Eliminate redundant clones #377

patrickfreed commented Jun 23, 2021 •

edited

Loading

patrickfreed Jun 23, 2021

patrickfreed Jun 23, 2021

patrickfreed Jun 23, 2021

abr-egn Jun 24, 2021

patrickfreed Jun 24, 2021

patrickfreed Jun 23, 2021

abr-egn Jun 24, 2021

patrickfreed Jun 24, 2021

patrickfreed Jun 23, 2021

patrickfreed Jun 23, 2021

patrickfreed Jun 23, 2021

patrickfreed Jun 23, 2021

abr-egn Jun 24, 2021

patrickfreed Jun 24, 2021

patrickfreed Jun 23, 2021

patrickfreed Jun 23, 2021

abr-egn Jun 24, 2021

abr-egn Jun 24, 2021

abr-egn Jun 24, 2021

patrickfreed Jun 24, 2021

abr-egn Jun 24, 2021

RUST-536 Eliminate redundant clones #377

RUST-536 Eliminate redundant clones #377

Conversation

patrickfreed commented Jun 23, 2021 • 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

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

patrickfreed commented Jun 23, 2021 •

edited

Loading