Take document by reference in PackedDocument::pack (#35570)

goffrie · Convex, Inc. · commit b726995c4b0f · 2025-03-19T18:57:22.000Z
This eliminates a few document clones.

GitOrigin-RevId: 7625f4e332690b955ab27c9054320d8f66fb6b87
diff --git a/crates/common/src/document.rs b/crates/common/src/document.rs
@@ -775,10 +775,10 @@ impl DeveloperDocument {
 pub struct PackedDocument(PackedValue<ByteBuffer>, ResolvedDocumentId);
 
 impl PackedDocument {
-    pub fn pack(document: ResolvedDocument) -> Self {
+    pub fn pack(document: &ResolvedDocument) -> Self {
         let document_id = document.id();
-        let value = document.document.into_value().0.into();
-        Self(PackedValue::pack(&value), document_id)
+        let value = document.document.value();
+        Self(PackedValue::pack_object(value), document_id)
     }
 
     pub fn unpack(&self) -> ResolvedDocument {
@@ -1133,7 +1133,7 @@ mod tests {
             let index_key_bytes = doc.index_key(&field_paths, ver).into_bytes();
             assert_eq!(
                 index_key_bytes,
-                *PackedDocument::pack(doc).index_key(
+                *PackedDocument::pack(&doc).index_key(
                     &field_paths, ver, &mut IndexKeyBuffer::new()
                 ),
             );
diff --git a/crates/database/benches/subscriptions.rs b/crates/database/benches/subscriptions.rs
@@ -136,7 +136,7 @@ fn load_datasets(
             let value = assert_obj!("body" => d.text);
             let creation_time = CreationTime::try_from(1.)?;
             let document = ResolvedDocument::new(id, creation_time, value)?;
-            documents.push(PackedDocument::pack(document));
+            documents.push(PackedDocument::pack(&document));
         }
 
         let terms_by_frequency: Vec<String> = frequency_map
diff --git a/crates/database/src/committer.rs b/crates/database/src/committer.rs
@@ -637,15 +637,15 @@ impl<RT: Runtime> Committer<RT> {
         }
         timer.finish();
 
-        let updates: Vec<_> = transaction.writes.into_coalesced_writes().collect();
+        let updates: Vec<_> = transaction.writes.coalesced_writes().collect();
         // The updates are ordered using table_dependency_sort_key,
         // which is the same order they should be applied to database metadata
         // and index data structures
         let mut ordered_updates = updates;
         ordered_updates.sort_by_key(|(id, update)| {
             table_dependency_sort_key(
                 BootstrapTableIds::new(&transaction.table_mapping),
-                InternalDocumentId::from(*id),
+                InternalDocumentId::from(**id),
                 update.new_document.as_ref(),
             )
         });
@@ -663,7 +663,7 @@ impl<RT: Runtime> Committer<RT> {
             commit_ts,
             ordered_updates
                 .into_iter()
-                .map(|(id, update)| (id, PackedDocumentUpdate::pack(update.into())))
+                .map(|(&id, update)| (id, PackedDocumentUpdate::pack(update)))
                 .collect(),
             write_source,
         );
@@ -679,7 +679,7 @@ impl<RT: Runtime> Committer<RT> {
     fn compute_writes(
         &self,
         commit_ts: Timestamp,
-        ordered_updates: &Vec<(ResolvedDocumentId, DocumentUpdateWithPrevTs)>,
+        ordered_updates: &Vec<(&ResolvedDocumentId, &DocumentUpdateWithPrevTs)>,
     ) -> anyhow::Result<(
         Vec<ValidatedDocumentWrite>,
         BTreeSet<(Timestamp, DatabaseIndexUpdate)>,
@@ -691,7 +691,7 @@ impl<RT: Runtime> Committer<RT> {
         // same tables and indexes as the base snapshot and the final publishing
         // snapshot. Therefore index writes can be computed from the current snapshot.
         let mut current_snapshot = self.snapshot_manager.read().latest_snapshot();
-        for (id, document_update) in ordered_updates.iter() {
+        for &(id, document_update) in ordered_updates.iter() {
             let (updates, doc_in_vector_index) =
                 current_snapshot.update(document_update, commit_ts)?;
             index_writes.extend(updates);
diff --git a/crates/database/src/reads.rs b/crates/database/src/reads.rs
@@ -887,7 +887,7 @@ mod tests {
         let doc_without_word = create_document_with_one_field(id, field_name, val!(document_text))?;
         Ok(read_set
             .overlaps(
-                &PackedDocument::pack(doc_without_word),
+                &PackedDocument::pack(&doc_without_word),
                 PersistenceVersion::default(),
             )
             .is_some())
@@ -924,7 +924,7 @@ mod tests {
         assert_eq!(
             read_set
                 .overlaps(
-                    &PackedDocument::pack(doc_with_word),
+                    &PackedDocument::pack(&doc_with_word),
                     PersistenceVersion::default()
                 )
                 .unwrap()
@@ -940,7 +940,7 @@ mod tests {
         )?;
         assert_eq!(
             read_set.overlaps(
-                &PackedDocument::pack(doc_without_word),
+                &PackedDocument::pack(&doc_without_word),
                 PersistenceVersion::default()
             ),
             None
@@ -980,7 +980,7 @@ mod tests {
         assert_eq!(
             read_set
                 .overlaps(
-                    &PackedDocument::pack(doc_with_explicit_null),
+                    &PackedDocument::pack(&doc_with_explicit_null),
                     PersistenceVersion::default()
                 )
                 .unwrap()
@@ -996,7 +996,7 @@ mod tests {
         )?;
         assert_eq!(
             read_set.overlaps(
-                &PackedDocument::pack(doc_with_missing_field),
+                &PackedDocument::pack(&doc_with_missing_field),
                 PersistenceVersion::default()
             ),
             None
@@ -1010,7 +1010,7 @@ mod tests {
         )?;
         assert_eq!(
             read_set.overlaps(
-                &PackedDocument::pack(doc_with_implicit_null),
+                &PackedDocument::pack(&doc_with_implicit_null),
                 PersistenceVersion::default()
             ),
             None
@@ -1057,7 +1057,7 @@ mod tests {
         assert_eq!(
             read_set
                 .overlaps(
-                    &PackedDocument::pack(doc_with_explicit_null),
+                    &PackedDocument::pack(&doc_with_explicit_null),
                     PersistenceVersion::default()
                 )
                 .unwrap()
@@ -1073,7 +1073,7 @@ mod tests {
         )?;
         assert_eq!(
             read_set.overlaps(
-                &PackedDocument::pack(doc_with_missing_field),
+                &PackedDocument::pack(&doc_with_missing_field),
                 PersistenceVersion::default()
             ),
             None
@@ -1087,7 +1087,7 @@ mod tests {
         )?;
         assert_eq!(
             read_set.overlaps(
-                &PackedDocument::pack(doc_with_implicit_null),
+                &PackedDocument::pack(&doc_with_implicit_null),
                 PersistenceVersion::default()
             ),
             None
diff --git a/crates/database/src/subscription.rs b/crates/database/src/subscription.rs
@@ -710,7 +710,7 @@ mod tests {
                 );
                 assert_eq!(*index_name.table(), id.tablet_id);
 
-                let document = pack(create_document(
+                let document = pack(&create_document(
                     query.field_path.clone(),
                     match &query.term {
                         TextQueryTerm::Exact(term) => term.clone(),
@@ -725,7 +725,7 @@ mod tests {
         result
     }
 
-    fn pack(doc: ResolvedDocument) -> PackedDocument {
+    fn pack(doc: &ResolvedDocument) -> PackedDocument {
         PackedDocument::pack(doc)
     }
 
diff --git a/crates/database/src/tests/mod.rs b/crates/database/src/tests/mod.rs
@@ -2280,7 +2280,7 @@ async fn test_query_filter_readset(rt: TestRuntime) -> anyhow::Result<()> {
     assert!(token
         .reads()
         .overlaps(
-            &PackedDocument::pack(out_of_range_doc),
+            &PackedDocument::pack(&out_of_range_doc),
             PersistenceVersion::default()
         )
         .is_none());
@@ -2289,7 +2289,7 @@ async fn test_query_filter_readset(rt: TestRuntime) -> anyhow::Result<()> {
     assert!(token
         .reads()
         .overlaps(
-            &PackedDocument::pack(in_range_doc),
+            &PackedDocument::pack(&in_range_doc),
             PersistenceVersion::default()
         )
         .is_some());
@@ -2355,7 +2355,7 @@ async fn test_query_readset_empty_query(rt: TestRuntime) -> anyhow::Result<()> {
     assert!(token
         .reads()
         .overlaps(
-            &PackedDocument::pack(in_range_doc),
+            &PackedDocument::pack(&in_range_doc),
             PersistenceVersion::default()
         )
         .is_some());
diff --git a/crates/database/src/transaction_index.rs b/crates/database/src/transaction_index.rs
@@ -513,7 +513,7 @@ impl TransactionIndex {
                     match new_document {
                         Some(ref doc) => {
                             assert_eq!(doc.id(), *doc_id);
-                            Some(doc.clone())
+                            Some(doc)
                         },
                         None => panic!("Unexpected index update: {:?}", update.value),
                     }
@@ -647,7 +647,7 @@ impl TransactionIndexMap {
             .map(|(k, v)| (IndexKeyBytes(k.clone()), v.as_ref().map(|v| v.unpack())))
     }
 
-    pub fn insert(&mut self, k: IndexKey, v: Option<ResolvedDocument>) {
+    pub fn insert(&mut self, k: IndexKey, v: Option<&ResolvedDocument>) {
         self.inner
             .insert(k.into_bytes().0, v.map(PackedDocument::pack));
     }
diff --git a/crates/database/src/write_log.rs b/crates/database/src/write_log.rs
@@ -11,6 +11,7 @@ use std::{
 use common::{
     document::{
         DocumentUpdate,
+        DocumentUpdateRef,
         PackedDocument,
     },
     knobs::{
@@ -61,11 +62,11 @@ impl HeapSize for PackedDocumentUpdate {
 type OrderedWrites = WithHeapSize<Vec<(ResolvedDocumentId, PackedDocumentUpdate)>>;
 
 impl PackedDocumentUpdate {
-    pub fn pack(update: DocumentUpdate) -> Self {
+    pub fn pack(update: &impl DocumentUpdateRef) -> Self {
         Self {
-            id: update.id,
-            old_document: update.old_document.map(PackedDocument::pack),
-            new_document: update.new_document.map(PackedDocument::pack),
+            id: update.id(),
+            old_document: update.old_document().map(PackedDocument::pack),
+            new_document: update.new_document().map(PackedDocument::pack),
         }
     }
 
@@ -635,7 +636,7 @@ mod tests {
             Timestamp::must(1003),
             vec![(
                 id,
-                PackedDocumentUpdate::pack(DocumentUpdate {
+                PackedDocumentUpdate::pack(&DocumentUpdate {
                     id,
                     old_document: None,
                     new_document: Some(doc.clone()),
@@ -749,7 +750,7 @@ mod tests {
             Timestamp::must(1003),
             vec![(
                 id,
-                PackedDocumentUpdate::pack(DocumentUpdate {
+                PackedDocumentUpdate::pack(&DocumentUpdate {
                     id,
                     old_document: Some(doc),
                     new_document: None,
diff --git a/crates/database/src/writes.rs b/crates/database/src/writes.rs
@@ -484,7 +484,7 @@ mod tests {
             user_table1.tablet_id,
         )?;
 
-        let user_table1_table_metadata_change = PackedDocument::pack(ResolvedDocument::new(
+        let user_table1_table_metadata_change = PackedDocument::pack(&ResolvedDocument::new(
             bootstrap_tables.table_resolved_doc_id(user_table1.tablet_id),
             CreationTime::ONE,
             TableMetadata::new(
@@ -502,7 +502,7 @@ mod tests {
             )
             .is_some());
 
-        let user_table1_index_change = PackedDocument::pack(ResolvedDocument::new(
+        let user_table1_index_change = PackedDocument::pack(&ResolvedDocument::new(
             id_generator.system_generate(&INDEX_TABLE),
             CreationTime::ONE,
             IndexMetadata::new_backfilling(
@@ -519,7 +519,7 @@ mod tests {
 
         // Writes to a table should *not* OCC with modification of the table metadata
         // or an index of unrelated same table.
-        let user_table2_table_metadata_change = PackedDocument::pack(ResolvedDocument::new(
+        let user_table2_table_metadata_change = PackedDocument::pack(&ResolvedDocument::new(
             bootstrap_tables.table_resolved_doc_id(user_table2.tablet_id),
             CreationTime::ONE,
             TableMetadata::new(
@@ -537,7 +537,7 @@ mod tests {
             )
             .is_none());
 
-        let user_table2_index_change = PackedDocument::pack(ResolvedDocument::new(
+        let user_table2_index_change = PackedDocument::pack(&ResolvedDocument::new(
             id_generator.system_generate(&INDEX_TABLE),
             CreationTime::ONE,
             IndexMetadata::new_backfilling(
diff --git a/crates/function_runner/src/in_memory_indexes.rs b/crates/function_runner/src/in_memory_indexes.rs
@@ -173,7 +173,7 @@ async fn load_index(
             Order::Asc,
             usize::MAX,
         )
-        .map_ok(|(key, rev)| (key.0, (rev.ts, PackedDocument::pack(rev.value))))
+        .map_ok(|(key, rev)| (key.0, (rev.ts, PackedDocument::pack(&rev.value))))
         .try_collect()
         .await?;
     log_funrun_index_load_rows(index_map.len() as u64, &table_name, &instance_name);
diff --git a/crates/indexing/src/backend_in_memory_indexes.rs b/crates/indexing/src/backend_in_memory_indexes.rs
@@ -122,7 +122,7 @@ impl BackendInMemoryIndexes {
         let mut meta_index_map = DatabaseIndexMap::new_at(ts);
         for (ts, index_doc) in index_documents.into_values() {
             let index_key = IndexKey::new(vec![], index_doc.developer_id());
-            meta_index_map.insert(index_key.into_bytes(), ts, index_doc);
+            meta_index_map.insert(index_key.into_bytes(), ts, &index_doc);
         }
 
         let mut in_memory_indexes = OrdMap::new();
@@ -215,7 +215,7 @@ impl BackendInMemoryIndexes {
         for (key, rev) in entries.into_iter() {
             num_keys += 1;
             total_size += rev.value.value().size();
-            index_map.insert(key, rev.ts, rev.value);
+            index_map.insert(key, rev.ts, &rev.value);
         }
 
         self.in_memory_indexes.insert(index.id(), index_map);
@@ -254,7 +254,7 @@ impl BackendInMemoryIndexes {
                         match insertion {
                             Some(ref doc) => {
                                 assert_eq!(*doc_id, doc.id());
-                                key_set.insert(update.key.clone().into_bytes(), ts, doc.clone());
+                                key_set.insert(update.key.clone().into_bytes(), ts, doc);
                             },
                             None => panic!("Unexpected index update: {:?}", update.value),
                         }
@@ -320,7 +320,7 @@ impl DatabaseIndexMap {
             .map(|(k, (ts, v))| (IndexKeyBytes(k.clone()), *ts, v.unpack()))
     }
 
-    fn insert(&mut self, k: IndexKeyBytes, ts: Timestamp, v: ResolvedDocument) {
+    fn insert(&mut self, k: IndexKeyBytes, ts: Timestamp, v: &ResolvedDocument) {
         self.inner.insert(k.0, (ts, PackedDocument::pack(v)));
         self.last_modified = cmp::max(self.last_modified, ts);
     }
diff --git a/crates/packed_value/src/lib.rs b/crates/packed_value/src/lib.rs
@@ -21,6 +21,7 @@ use flexbuffers::{
 };
 use value::{
     heap_size::HeapSize,
+    ConvexObject,
     ConvexValue,
     FieldPath,
 };
@@ -122,6 +123,14 @@ impl PackedValue<ByteBuffer> {
         }
     }
 
+    pub fn pack_object(value: &ConvexObject) -> Self {
+        let mut builder = Builder::default();
+        Self::_pack_object(value, &mut builder);
+        Self {
+            buf: builder.take_buffer().into(),
+        }
+    }
+
     fn _pack(value: &ConvexValue, builder: &mut impl FlexBuilder) {
         match value {
             ConvexValue::Null => {
@@ -169,15 +178,19 @@ impl PackedValue<ByteBuffer> {
                 map.end_map();
             },
             ConvexValue::Object(ref fields) => {
-                let mut map = builder.start_map();
-                for (field, value) in fields.iter() {
-                    let mut builder = (&field[..], &mut map);
-                    Self::_pack(value, &mut builder);
-                }
-                map.end_map();
+                Self::_pack_object(fields, builder);
             },
         }
     }
+
+    fn _pack_object(object: &ConvexObject, builder: &mut impl FlexBuilder) {
+        let mut map = builder.start_map();
+        for (field, value) in object.iter() {
+            let mut builder = (&field[..], &mut map);
+            Self::_pack(value, &mut builder);
+        }
+        map.end_map();
+    }
 }
 
 pub enum OpenedValue<B: Buffer = ByteBuffer>

Original file line number	Diff line number	Diff line change
`@@ -136,7 +136,7 @@ fn load_datasets(`
`136`	`136`	`let value = assert_obj!("body" => d.text);`
`137`	`137`	`let creation_time = CreationTime::try_from(1.)?;`
`138`	`138`	`let document = ResolvedDocument::new(id, creation_time, value)?;`
`139`		`- documents.push(PackedDocument::pack(document));`
	`139`	`+ documents.push(PackedDocument::pack(&document));`
`140`	`140`	`}`
`141`	`141`
`142`	`142`	`let terms_by_frequency: Vec<String> = frequency_map`
Original file line number	Diff line number	Diff line change
`@@ -710,7 +710,7 @@ mod tests {`
`710`	`710`	`);`
`711`	`711`	`assert_eq!(*index_name.table(), id.tablet_id);`
`712`	`712`
`713`		`- let document = pack(create_document(`
	`713`	`+ let document = pack(&create_document(`
`714`	`714`	`query.field_path.clone(),`
`715`	`715`	`match &query.term {`
`716`	`716`	`TextQueryTerm::Exact(term) => term.clone(),`
`@@ -725,7 +725,7 @@ mod tests {`
`725`	`725`	`result`
`726`	`726`	`}`
`727`	`727`
`728`		`- fn pack(doc: ResolvedDocument) -> PackedDocument {`
	`728`	`+ fn pack(doc: &ResolvedDocument) -> PackedDocument {`
`729`	`729`	`PackedDocument::pack(doc)`
`730`	`730`	`}`
`731`	`731`
Original file line number	Diff line number	Diff line change
`@@ -513,7 +513,7 @@ impl TransactionIndex {`
`513`	`513`	`match new_document {`
`514`	`514`	`Some(ref doc) => {`
`515`	`515`	`assert_eq!(doc.id(), *doc_id);`
`516`		`- Some(doc.clone())`
	`516`	`+ Some(doc)`
`517`	`517`	`},`
`518`	`518`	`None => panic!("Unexpected index update: {:?}", update.value),`
`519`	`519`	`}`
`@@ -647,7 +647,7 @@ impl TransactionIndexMap {`
`647`	`647`	`.map(\|(k, v)\| (IndexKeyBytes(k.clone()), v.as_ref().map(\|v\| v.unpack())))`
`648`	`648`	`}`
`649`	`649`
`650`		`- pub fn insert(&mut self, k: IndexKey, v: Option<ResolvedDocument>) {`
	`650`	`+ pub fn insert(&mut self, k: IndexKey, v: Option<&ResolvedDocument>) {`
`651`	`651`	`self.inner`
`652`	`652`	`.insert(k.into_bytes().0, v.map(PackedDocument::pack));`
`653`	`653`	`}`
Original file line number	Diff line number	Diff line change
`@@ -173,7 +173,7 @@ async fn load_index(`
`173`	`173`	`Order::Asc,`
`174`	`174`	`usize::MAX,`
`175`	`175`	`)`
`176`		`- .map_ok(\|(key, rev)\| (key.0, (rev.ts, PackedDocument::pack(rev.value))))`
	`176`	`+ .map_ok(\|(key, rev)\| (key.0, (rev.ts, PackedDocument::pack(&rev.value))))`
`177`	`177`	`.try_collect()`
`178`	`178`	`.await?;`
`179`	`179`	`log_funrun_index_load_rows(index_map.len() as u64, &table_name, &instance_name);`