RUST-1175 Support zero-copy deserialization from cursors #579

patrickfreed · 2022-02-22T20:38:49Z

This PR adds new methods to Cursor and SessionCursor to allow for zero-copy deserialization. As part of that, the trait requirements on the cursor types were relaxed, and they no longer require DeserializeOwned. The Stream implementations still do require all of the trait requirements though.

example usage:

#[derive(Debug, Deserialize)]
struct Stuff<'a> {
    #[serde(borrow)]
    name: &'a str,
    #[serde(borrow)]
    bio: &'a str
}

let coll = db.collection::<Stuff>("stuff");
let mut cursor = coll.find(None, None).await?;
while cursor.advance()?.await {
    println!("{:?}", cursor.deserialize_current()?);
}

patrickfreed · 2022-02-22T20:40:01Z

src/cursor/common.rs

@@ -37,17 +37,15 @@ where
    provider: P,
    client: Client,
    info: CursorInformation,
-    buffer: VecDeque<RawDocumentBuf>,


I moved the stuff that needed to be preserved between GenericCursor constructions in a CursorState struct to ensure nothing gets forgotten.

patrickfreed · 2022-02-22T20:41:32Z

src/cursor/mod.rs

@@ -90,10 +91,7 @@ pub(crate) use common::{
 /// # }
 /// ```
 #[derive(Debug)]
-pub struct Cursor<T>
-where
-    T: DeserializeOwned + Unpin + Send + Sync,


Since we no longer buffer T, we can drop the Unpin + Send + Sync parts of this, and because we now support borrowed deserialization, we can drop DeserializeOwned too.

patrickfreed · 2022-02-22T20:42:59Z

src/operation/aggregate/test.rs

@@ -188,59 +186,6 @@ async fn op_selection_criteria() {
    });
 }

-#[cfg_attr(feature = "tokio-runtime", tokio::test)]
-#[cfg_attr(feature = "async-std-runtime", async_std::test)]
-async fn handle_success() {


Rather than continuing to maintain these tests that break a lot and don't offer much additional coverage over our integration tests, I decided just to delete the ones that broke. We discussed this a bit in slack, but in retrospect these tests are probably far too close to the actual implementation and are probably more effort than they're worth. Thoughts?

agreed, I don't think these are particularly valuable

isabelatkinson

looks good, just a few comments. do we also want to add this functionality to the sync API?

isabelatkinson · 2022-02-23T18:47:05Z

src/cursor/common.rs

+    }
+
+    fn handle_get_more_result(&mut self, get_more_result: Result<GetMoreResult>) -> Result<()> {
+        self.state_mut().exhausted = get_more_result


is this necessary given mark_exhausted is called below?

nope, good catch. removed

isabelatkinson · 2022-02-23T19:29:53Z

src/operation/aggregate/test.rs

@@ -188,59 +186,6 @@ async fn op_selection_criteria() {
    });
 }

-#[cfg_attr(feature = "tokio-runtime", tokio::test)]
-#[cfg_attr(feature = "async-std-runtime", async_std::test)]
-async fn handle_success() {


agreed, I don't think these are particularly valuable

patrickfreed

Ah yep, totally forgot the sync API. Added it it now

patrickfreed · 2022-02-23T20:28:30Z

src/cursor/common.rs

+    }
+
+    fn handle_get_more_result(&mut self, get_more_result: Result<GetMoreResult>) -> Result<()> {
+        self.state_mut().exhausted = get_more_result


nope, good catch. removed

patrickfreed · 2022-02-23T20:30:46Z

src/cursor/session.rs

    fn mark_exhausted(&mut self) {
-        self.info.id = 0;
+        self.state.as_mut().unwrap().exhausted = true;


now that state is the single source of truth, this needed to be updated to use exhausted instead of the spec.

abr-egn

LGTM! Minor comments only.

abr-egn · 2022-02-28T14:34:06Z

src/cursor/common.rs

    }

    pub(super) fn is_exhausted(&self) -> bool {
-        self.exhausted
+        self.state.as_ref().unwrap().exhausted


For this and similar calls, I think it would be slightly better to centralize the unwrap in the state() accessor and write this as self.state().exhausted.

Updated. I had to leave the one on line 245 in to satisfy the borrow checker, not sure why though.

abr-egn · 2022-02-28T14:44:43Z

src/operation/aggregate/test.rs

@@ -188,59 +186,6 @@ async fn op_selection_criteria() {
    });
 }

-#[cfg_attr(feature = "tokio-runtime", tokio::test)]
-#[cfg_attr(feature = "async-std-runtime", async_std::test)]
-async fn handle_success() {


patrickfreed added 14 commits February 22, 2022 14:46

first pass no copy deserialization

255e017

wip session

a20536d

finish session?

57ee2c2

redo counts

9d39e4c

wip state

b3180b0

fix fmt, clippy, state

c5e6ace

wip new api

5090664

finish new api and docs

faea67c

add test

a442381

use exh

511f926

use vecdeque for cursorbuffer

fddf287

improve docs

e20bdd9

fix clippy

bd9246b

remove error caching, properly check if cursor is exhausted

790850e

patrickfreed commented Feb 22, 2022

View reviewed changes

patrickfreed requested a review from isabelatkinson February 22, 2022 20:54

isabelatkinson reviewed Feb 23, 2022

View reviewed changes

patrickfreed added 6 commits February 23, 2022 14:46

stop using spec.id to test if cursor is exhausted in sessioncursors

e6bb4a0

remove redundant exhausted mark

6e4b5ce

add borrowed deserialization sync cursor api

b69642b

update async borrowed deserialization test to cover session cursors

6123aa8

fix clippy

36f6a06

fix rustfmt

c58828a

patrickfreed commented Feb 23, 2022

View reviewed changes

patrickfreed requested a review from isabelatkinson February 23, 2022 20:31

fix sync doc tests

ff4d14f

isabelatkinson approved these changes Feb 24, 2022

View reviewed changes

abr-egn approved these changes Feb 28, 2022

View reviewed changes

use state accessor

717cd39

patrickfreed merged commit 8e07cf4 into mongodb:master Feb 28, 2022

RUST-1175 Support zero-copy deserialization from cursors #579

RUST-1175 Support zero-copy deserialization from cursors #579

Uh oh!

Conversation

patrickfreed commented Feb 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isabelatkinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickfreed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abr-egn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!