[fastx db] Update an owner-object index #149

gdanezis · 2022-01-10T18:51:08Z

This addresses issue #127

We add and owner_index to the database.
We update the index when we insert an object, or the effects of a transaction.
To make this a little more efficient we record in the transaction processing handler the old owners of deleted or mutated objects, to know which old index entries to delete.
Now use multi-get where possible, and minimise reads by generating the OrderInfoResponse objects.

huitseeker

This LGTM, but I've left a few Qs.

huitseeker · 2022-01-11T13:43:47Z

fastpay_core/src/authority.rs

+        // TODO: We only need to read the read_only and owner field of the object,
+        //      it's a bit wasteful to copy the entire object.
+        let _objects = self.get_objects(&ids[..]).await?;


Is it faster to .get_objects or could you get_object by reference and only copy what you need?

I do not think there is an option of getting an object by reference from the DB right now. As soon as we do get, the object is read from the db value (bytes are pinned) and then deserialized and allocated as a whole. I went down the rabbit hole of looking at serialization formats that allow cheap in place reads (zero copy), like capnpn, flatbuf etc -- but changing the DB to use those is a bigger job.

huitseeker · 2022-01-11T13:44:22Z

fastpay_core/src/authority.rs

+            // let object = self
+            //     .object_state(&object_id)
+            //     .await
+            //    .map_err(|_| FastPayError::ObjectNotFound)?;


Does the commented code still provide value?

Nice catch, that was just forgotten.

huitseeker · 2022-01-11T13:45:41Z

fastpay_core/src/authority.rs

+        let input_objects = order.input_objects();
+        let ids: Vec<_> = input_objects.iter().map(|(id, _, _)| *id).collect();
+        // Get a copy of the object.
+        // TODO: We only need to read the read_only and owner field of the object,
+        //      it's a bit wasteful to copy the entire object.
+        let _objects = self.get_objects(&ids[..]).await?;
+
        let mut inputs = vec![];
-        for (input_object_id, input_seq, _input_digest) in order.input_objects() {
+        let mut owner_index = HashMap::new();
+        for (object_ref, object) in input_objects.into_iter().zip(_objects) {
+            let (input_object_id, input_seq, _input_digest) = object_ref;


Nit: you may want to extract a private function to cover this and the above.

My iterator-foo is not up to defining a function with a generic return signature of an iterator :)

huitseeker · 2022-01-11T13:47:13Z

fastpay_core/src/authority.rs

+                    .filter(|(id, _, _)| {
+                        let owner = owner_index.get(id);
+                        owner.is_some() && *owner.unwrap() != temporary_store.objects()[id].owner
+                    })
+                    .map(|(id, _, _)| (owner_index[id], *id)),


Nit: consider filter_map

Nice to know this exists.

huitseeker · 2022-01-11T13:49:46Z

fastpay_core/src/authority/authority_store.rs

+                &self.owner_index,
+                _expired_object_owners
+                    .into_iter()
+                    .map(|(owner, id)| (owner, id)),


Why the map?

No reason any more, removed!

huitseeker · 2022-01-11T13:50:48Z

fastpay_core/src/authority/temporary_store.rs

@@ -26,6 +26,20 @@ impl AuthorityTemporaryStore {
        }
    }

+    // Helpers to access private fields


Nit: You might like readonly.

I like fewer dependencies :)

huitseeker · 2022-01-11T13:56:04Z

fastpay_core/src/authority/authority_store.rs

@@ -9,6 +9,7 @@ use typed_store::traits::Map;
 pub struct AuthorityStore {
    objects: DBMap<ObjectID, Object>,
    order_lock: DBMap<ObjectRef, Option<TransactionDigest>>,
+    owner_index: DBMap<(FastPayAddress, ObjectID), ObjectRef>,


Nice way to get out of the fact that it's unwieldy to update collections inside the DB ... but this is something that may require comment, either here or around get_objects:
DBMap<(FastPayAddress, ObjectID), ObjectRef> has far greater machine affinity than DBMap<FastPayAddress, SomeCollectionOf<ObjectRef>>,

This is the standard way of using a persistent BTree to make a secondary index, I did not invent it :). Just to check -- I think our choice of serialization (bincode with fixint, bigendian, etc) was specifically selected to allow for these "composite" indexes. Am I correct in thinking this is true? It is not the case that this will break in a different architecture, right?

I think our choice of serialization (bincode with fixint, bigendian, etc) was specifically selected to allow for these "composite" indexes. Am I correct in thinking this is true? It is not the case that this will break in a different architecture, right?

Yes, this is correct.

patrickkuo

Some commented out code and _ prefix, otherwise LGTM

patrickkuo · 2022-01-11T13:44:14Z

fastpay_core/src/authority.rs

-                .object_state(&object_id)
-                .await
-                .map_err(|_| FastPayError::ObjectNotFound)?;
+            // let object = self


can we remove comment out code?

patrickkuo · 2022-01-11T13:45:35Z

fastpay_core/src/authority.rs

+        // Get a copy of the object.
+        // TODO: We only need to read the read_only and owner field of the object,
+        //      it's a bit wasteful to copy the entire object.
+        let _objects = self.get_objects(&ids[..]).await?;


why _ prefix?

I suspect https://rust-lang.github.io/rust-clippy/master/#used_underscore_binding

Now the object is used so removed the _ indeed

patrickkuo · 2022-01-11T13:48:29Z

fastpay_core/src/authority.rs

+        // Get a copy of the object.
+        // TODO: We only need to read the read_only and owner field of the object,
+        //      it's a bit wasteful to copy the entire object.
+        let _objects = self.get_objects(&ids[..]).await?;


patrickkuo · 2022-01-11T13:53:50Z

fastpay_core/src/authority/authority_store.rs

@@ -221,9 +242,10 @@ impl AuthorityStore {
    pub fn update_state(
        &self,
        temporary_store: AuthorityTemporaryStore,
+        _expired_object_owners: Vec<(FastPayAddress, ObjectID)>,


don't need _ prefix?

patrickkuo · 2022-01-11T13:58:51Z

fastpay_core/src/authority.rs

+        )
+        .await
+
+        // self.make_order_info(&transaction_digest).await


is this intended?

Nope, removed and commented instead.

oxade · 2022-01-11T15:37:36Z

Besides the minor comments, LGTM!
Thanks for the PR @gdanezis

gdanezis force-pushed the index-object-owner branch from 369b3ca to 7f8a8ef Compare January 10, 2022 19:48

gdanezis changed the title ~~Update an owner-object index~~ [fastx db] Update an owner-object index Jan 10, 2022

gdanezis linked an issue Jan 10, 2022 that may be closed by this pull request

[fastx distsys] Add Object Owner index to AuthorityStore #127

Closed

gdanezis self-assigned this Jan 10, 2022

gdanezis requested a review from patrickkuo January 10, 2022 19:48

gdanezis force-pushed the index-object-owner branch from 7f8a8ef to 3b9c514 Compare January 10, 2022 22:12

George Danezis added 4 commits January 10, 2022 23:41

Upate an owener-object index

3a70b9f

Remove _

d5e3df7

Fmt & comments

962810c

Use multi-get from db

80b5c6e

gdanezis force-pushed the index-object-owner branch from adca8c2 to 80b5c6e Compare January 10, 2022 23:42

huitseeker approved these changes Jan 11, 2022

View reviewed changes

patrickkuo approved these changes Jan 11, 2022

View reviewed changes

Clean up after reviews

3dc355d

gdanezis merged commit 9399897 into main Jan 11, 2022

gdanezis deleted the index-object-owner branch January 11, 2022 16:40

This was referenced Apr 5, 2024

[Snyk] Upgrade class-variance-authority from 0.4.0 to 0.7.0 Ankitpa2927/sui#10

Open

[Snyk] Upgrade class-variance-authority from 0.4.0 to 0.7.0 Ankitpa2927/sui#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fastx db] Update an owner-object index #149

[fastx db] Update an owner-object index #149

gdanezis commented Jan 10, 2022 •

edited

Loading

huitseeker left a comment

huitseeker Jan 11, 2022

gdanezis Jan 11, 2022

huitseeker Jan 11, 2022

gdanezis Jan 11, 2022

huitseeker Jan 11, 2022

gdanezis Jan 11, 2022

huitseeker Jan 11, 2022

gdanezis Jan 11, 2022

huitseeker Jan 11, 2022

gdanezis Jan 11, 2022

huitseeker Jan 11, 2022

gdanezis Jan 11, 2022

huitseeker Jan 11, 2022

gdanezis Jan 11, 2022

huitseeker Jan 12, 2022

patrickkuo left a comment

patrickkuo Jan 11, 2022

patrickkuo Jan 11, 2022

huitseeker Jan 11, 2022

gdanezis Jan 11, 2022

patrickkuo Jan 11, 2022

patrickkuo Jan 11, 2022

gdanezis Jan 11, 2022

patrickkuo Jan 11, 2022

gdanezis Jan 11, 2022

oxade commented Jan 11, 2022 •

edited

Loading

[fastx db] Update an owner-object index #149

[fastx db] Update an owner-object index #149

Conversation

gdanezis commented Jan 10, 2022 • edited Loading

huitseeker 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

Choose a reason for hiding this comment

patrickkuo 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

oxade commented Jan 11, 2022 • edited Loading

gdanezis commented Jan 10, 2022 •

edited

Loading

oxade commented Jan 11, 2022 •

edited

Loading