Resource Groups: don't recompute size, test serialized size, parallel…

… finalization (#14489) * Resource Groups: don't recompute size, test serialized size * Moving finalizing groups to (parallel) materialization stage
aptos-labs · Oct 4, 2024 · d9c7970 · d9c7970
1 parent bf7f6fb
commit d9c7970
Show file tree

Hide file tree

Showing 23 changed files with 1,463 additions and 1,198 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/aptos-move/aptos-vm-types/src/resource_group_adapter.rs b/aptos-move/aptos-vm-types/src/resource_group_adapter.rs
@@ -3,7 +3,8 @@
 
 use crate::resolver::{ResourceGroupSize, ResourceGroupView, TResourceGroupView, TResourceView};
 use aptos_types::{
-    serde_helper::bcs_utils::bcs_size_of_byte_array, state_store::state_key::StateKey,
+    error::code_invariant_error, serde_helper::bcs_utils::bcs_size_of_byte_array,
+    state_store::state_key::StateKey,
 };
 use bytes::Bytes;
 use move_binary_format::errors::{PartialVMError, PartialVMResult};
@@ -273,6 +274,104 @@ impl TResourceGroupView for ResourceGroupAdapter<'_> {
     }
 }
 
+// We set SPECULATIVE_EXECUTION_ABORT_ERROR here, as the error can happen due to
+// speculative reads (and in a non-speculative context, e.g. during commit, it
+// is a more serious error and block execution must abort).
+// BlockExecutor is responsible with handling this error.
+fn group_size_arithmetics_error() -> PartialVMError {
+    PartialVMError::new(StatusCode::SPECULATIVE_EXECUTION_ABORT_ERROR)
+        .with_message("Group size arithmetics error while applying updates".to_string())
+}
+
+// Updates a given ResourceGroupSize (an abstract representation allowing the computation
+// of bcs serialized size) size, to reflect the state after removing a resource in a group
+// with size old_tagged_resource_size.
+pub fn decrement_size_for_remove_tag(
+    size: &mut ResourceGroupSize,
+    old_tagged_resource_size: u64,
+) -> PartialVMResult<()> {
+    match size {
+        ResourceGroupSize::Concrete(_) => Err(code_invariant_error(format!(
+            "Unexpected ResourceGroupSize::Concrete in decrement_size_for_add_tag \
+	     (removing resource w. size = {old_tagged_resource_size})"
+        ))
+        .into()),
+        ResourceGroupSize::Combined {
+            num_tagged_resources,
+            all_tagged_resources_size,
+        } => {
+            *num_tagged_resources = num_tagged_resources
+                .checked_sub(1)
+                .ok_or_else(group_size_arithmetics_error)?;
+            *all_tagged_resources_size = all_tagged_resources_size
+                .checked_sub(old_tagged_resource_size)
+                .ok_or_else(group_size_arithmetics_error)?;
+            Ok(())
+        },
+    }
+}
+
+// Updates a given ResourceGroupSize (an abstract representation allowing the computation
+// of bcs serialized size) size, to reflect the state after adding a resource in a group
+// with size new_tagged_resource_size.
+pub fn increment_size_for_add_tag(
+    size: &mut ResourceGroupSize,
+    new_tagged_resource_size: u64,
+) -> PartialVMResult<()> {
+    match size {
+        ResourceGroupSize::Concrete(_) => Err(code_invariant_error(format!(
+            "Unexpected ResourceGroupSize::Concrete in increment_size_for_add_tag \
+		     (adding resource w. size = {new_tagged_resource_size})"
+        ))
+        .into()),
+        ResourceGroupSize::Combined {
+            num_tagged_resources,
+            all_tagged_resources_size,
+        } => {
+            *num_tagged_resources = num_tagged_resources
+                .checked_add(1)
+                .ok_or_else(group_size_arithmetics_error)?;
+            *all_tagged_resources_size = all_tagged_resources_size
+                .checked_add(new_tagged_resource_size)
+                .ok_or_else(group_size_arithmetics_error)?;
+            Ok(())
+        },
+    }
+}
+
+// Checks an invariant that iff a resource group exists, it must have a > 0 size.
+pub fn check_size_and_existence_match(
+    size: &ResourceGroupSize,
+    exists: bool,
+    state_key: &StateKey,
+) -> PartialVMResult<()> {
+    if exists {
+        if size.get() == 0 {
+            Err(
+                PartialVMError::new(StatusCode::SPECULATIVE_EXECUTION_ABORT_ERROR).with_message(
+                    format!(
+                        "Group tag count/size shouldn't be 0 for an existing group: {:?}",
+                        state_key
+                    ),
+                ),
+            )
+        } else {
+            Ok(())
+        }
+    } else if size.get() > 0 {
+        Err(
+            PartialVMError::new(StatusCode::SPECULATIVE_EXECUTION_ABORT_ERROR).with_message(
+                format!(
+                    "Group tag count/size should be 0 for a new group: {:?}",
+                    state_key
+                ),
+            ),
+        )
+    } else {
+        Ok(())
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

diff --git a/aptos-move/aptos-vm/src/block_executor/mod.rs b/aptos-move/aptos-vm/src/block_executor/mod.rs
@@ -33,6 +33,7 @@ use aptos_types::{
 use aptos_vm_logging::{flush_speculative_logs, init_speculative_logs};
 use aptos_vm_types::{
     abstract_write_op::AbstractResourceWriteOp, environment::Environment, output::VMOutput,
+    resolver::ResourceGroupSize,
 };
 use move_core_types::{
     language_storage::StructTag,
@@ -118,6 +119,7 @@ impl BlockExecutorTransactionOutput for AptosTransactionOutput {
     ) -> Vec<(
         StateKey,
         WriteOp,
+        ResourceGroupSize,
         BTreeMap<StructTag, (WriteOp, Option<Arc<MoveTypeLayout>>)>,
     )> {
         self.vm_output
@@ -131,6 +133,9 @@ impl BlockExecutorTransactionOutput for AptosTransactionOutput {
                     Some((
                         key.clone(),
                         group_write.metadata_op().clone(),
+                        group_write
+                            .maybe_group_op_size()
+                            .unwrap_or(ResourceGroupSize::zero_combined()),
                         group_write
                             .inner_ops()
                             .iter()

diff --git a/aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs b/aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs
@@ -9,16 +9,18 @@ use aptos_types::{
     write_set::WriteOp,
 };
 use aptos_vm_types::{
-    abstract_write_op::GroupWrite, resolver::ResourceGroupSize,
-    resource_group_adapter::group_tagged_resource_size,
+    abstract_write_op::GroupWrite,
+    resource_group_adapter::{
+        check_size_and_existence_match, decrement_size_for_remove_tag, group_tagged_resource_size,
+        increment_size_for_add_tag,
+    },
 };
 use bytes::Bytes;
 use move_binary_format::errors::{PartialVMError, PartialVMResult};
 use move_core_types::{
     effects::Op as MoveStorageOp, language_storage::StructTag, value::MoveTypeLayout,
     vm_status::StatusCode,
 };
-use move_vm_types::delayed_values::error::code_invariant_error;
 use std::{collections::BTreeMap, sync::Arc};
 
 pub(crate) struct WriteOpConverter<'r> {
@@ -47,96 +49,6 @@ macro_rules! convert_impl {
     };
 }
 
-// We set SPECULATIVE_EXECUTION_ABORT_ERROR here, as the error can happen due to
-// speculative reads (and in a non-speculative context, e.g. during commit, it
-// is a more serious error and block execution must abort).
-// BlockExecutor is responsible with handling this error.
-fn group_size_arithmetics_error() -> PartialVMError {
-    PartialVMError::new(StatusCode::SPECULATIVE_EXECUTION_ABORT_ERROR)
-        .with_message("Group size arithmetics error while applying updates".to_string())
-}
-
-fn decrement_size_for_remove_tag(
-    size: &mut ResourceGroupSize,
-    old_tagged_resource_size: u64,
-) -> PartialVMResult<()> {
-    match size {
-        ResourceGroupSize::Concrete(_) => Err(code_invariant_error(
-            "Unexpected ResourceGroupSize::Concrete in decrement_size_for_remove_tag",
-        )),
-        ResourceGroupSize::Combined {
-            num_tagged_resources,
-            all_tagged_resources_size,
-        } => {
-            *num_tagged_resources = num_tagged_resources
-                .checked_sub(1)
-                .ok_or_else(group_size_arithmetics_error)?;
-            *all_tagged_resources_size = all_tagged_resources_size
-                .checked_sub(old_tagged_resource_size)
-                .ok_or_else(group_size_arithmetics_error)?;
-            Ok(())
-        },
-    }
-}
-
-fn increment_size_for_add_tag(
-    size: &mut ResourceGroupSize,
-    new_tagged_resource_size: u64,
-) -> PartialVMResult<()> {
-    match size {
-        ResourceGroupSize::Concrete(_) => Err(PartialVMError::new(
-            StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR,
-        )
-        .with_message(
-            "Unexpected ResourceGroupSize::Concrete in increment_size_for_add_tag".to_string(),
-        )),
-        ResourceGroupSize::Combined {
-            num_tagged_resources,
-            all_tagged_resources_size,
-        } => {
-            *num_tagged_resources = num_tagged_resources
-                .checked_add(1)
-                .ok_or_else(group_size_arithmetics_error)?;
-            *all_tagged_resources_size = all_tagged_resources_size
-                .checked_add(new_tagged_resource_size)
-                .ok_or_else(group_size_arithmetics_error)?;
-            Ok(())
-        },
-    }
-}
-
-fn check_size_and_existence_match(
-    size: &ResourceGroupSize,
-    exists: bool,
-    state_key: &StateKey,
-) -> PartialVMResult<()> {
-    if exists {
-        if size.get() == 0 {
-            Err(
-                PartialVMError::new(StatusCode::SPECULATIVE_EXECUTION_ABORT_ERROR).with_message(
-                    format!(
-                        "Group tag count/size shouldn't be 0 for an existing group: {:?}",
-                        state_key
-                    ),
-                ),
-            )
-        } else {
-            Ok(())
-        }
-    } else if size.get() > 0 {
-        Err(
-            PartialVMError::new(StatusCode::SPECULATIVE_EXECUTION_ABORT_ERROR).with_message(
-                format!(
-                    "Group tag count/size should be 0 for a new group: {:?}",
-                    state_key
-                ),
-            ),
-        )
-    } else {
-        Ok(())
-    }
-}
-
 impl<'r> WriteOpConverter<'r> {
     convert_impl!(convert_module, get_module_state_value_metadata);
 

diff --git a/aptos-move/block-executor/src/captured_reads.rs b/aptos-move/block-executor/src/captured_reads.rs
@@ -622,9 +622,6 @@ impl<T: Transaction> CapturedReads<T> {
                     Err(Uninitialized) => {
                         unreachable!("May not be uninitialized if captured for validation");
                     },
-                    Err(TagSerializationError(_)) => {
-                        unreachable!("Should not require tag serialization");
-                    },
                 }
             })
         })