Skip to content

Commit

Permalink
Simplify the code and test related to recovering state from partitions (
Browse files Browse the repository at this point in the history
#5441)

# Motivation

Simplify the code and test related to recovering state from partitions

# Changes

* The only test using `into_memory` is removed (along with
`into_memory`). The back-and-forth conversion between partitions and
memory doesn't happen in production.
* `Partitions::new` and `Partitions::from` are now the same, so just
keep the latter
* Inline Partitions->State conversion since the additional indirection
doesn't seem to help much.

# Tests

N/A, just refactoring

# Todos

- [x] Add entry to changelog (if necessary).
  • Loading branch information
jasonz-dfinity authored Sep 12, 2024
1 parent 016b7d9 commit 5159856
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 81 deletions.
21 changes: 6 additions & 15 deletions rs/backend/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ impl State {
/// Creates new state with the specified schema.
#[must_use]
pub fn new(memory: DefaultMemoryImpl) -> Self {
let partitions = Partitions::new(memory);
let partitions = Partitions::from(memory);
let accounts_store = AccountsStore::from(AccountsDb::UnboundedStableBTreeMap(
AccountsDbAsUnboundedStableBTreeMap::new(partitions.get(PartitionType::Accounts.memory_id())),
));
Expand All @@ -125,27 +125,18 @@ impl State {
}
}

/// Restores state from managed memory.
impl From<Partitions> for State {
fn from(partitions: Partitions) -> Self {
println!("START state::from<Partitions>: ()");
/// Restores state from stable memory.
impl From<DefaultMemoryImpl> for State {
fn from(memory: DefaultMemoryImpl) -> Self {
println!("START state::from<DefaultMemoryImpl>: ())");
let partitions = Partitions::from(memory);
let state = Self::recover_heap_from_managed_memory(&partitions.get(PartitionType::Heap.memory_id()));
let accounts_db = AccountsDb::UnboundedStableBTreeMap(AccountsDbAsUnboundedStableBTreeMap::load(
partitions.get(PartitionType::Accounts.memory_id()),
));
// Replace the default accountsdb created by `serde` with the one from stable memory.
let _deserialized_accounts_db = state.accounts_store.borrow_mut().replace_accounts_db(accounts_db);
state.partitions_maybe.replace(PartitionsMaybe::Partitions(partitions));
state
}
}

/// Restores state from stable memory.
impl From<DefaultMemoryImpl> for State {
fn from(memory: DefaultMemoryImpl) -> Self {
println!("START state::from<DefaultMemoryImpl>: ())");
let partitions = Partitions::from(memory);
let state = Self::from(partitions);
println!("END state::from<DefaultMemoryImpl>: ()");
state
}
Expand Down
13 changes: 0 additions & 13 deletions rs/backend/src/state/partitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ use ic_stable_structures::memory_manager::{MemoryId, MemoryManager, VirtualMemor
use ic_stable_structures::{DefaultMemoryImpl, Memory};
use strum::IntoEnumIterator;
use strum_macros::EnumIter;

pub mod schemas;
#[cfg(test)]
pub mod tests;

Expand Down Expand Up @@ -105,17 +103,6 @@ impl Partitions {
self.memory_manager.borrow().get(memory_id)
}

/// Returns the raw memory, discarding the partitions data structure in RAM.
///
/// Note: The memory manager is still represented in the underlying memory,
/// so converting from `Partitions` to `DefaultMemoryImpl` and back again
/// returns to the original state.
/// TODO: remove `into_memory` after simplifying `post_upgrade`.
#[cfg(test)]
pub fn into_memory(self) -> DefaultMemoryImpl {
self.memory_manager.into_memory().expect("No underlying memory")
}

/// Writes, growing the memory if necessary.
pub fn growing_write(&self, memory_id: MemoryId, offset: u64, bytes: &[u8]) {
let memory = self.get(memory_id);
Expand Down
18 changes: 0 additions & 18 deletions rs/backend/src/state/partitions/schemas.rs

This file was deleted.

36 changes: 1 addition & 35 deletions rs/backend/src/state/partitions/tests.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
//! Tests for stable memory layout code.
use super::*;
use ic_cdk::api::stable::WASM_PAGE_SIZE_IN_BYTES;
use ic_crypto_sha2::Sha256;
use pretty_assertions::assert_eq;
use std::rc::Rc;

Expand Down Expand Up @@ -100,39 +99,6 @@ fn partitions_should_get_correct_virtual_memory() {
// Basic sanity check seems OK!
}

#[test]
fn should_be_able_to_convert_memory_to_partitions_and_back() {
/// Memory hasher, used to check that the memory is the same before and after.
fn hash_memory(memory: &DefaultMemoryImpl) -> [u8; 32] {
let mut hasher = Sha256::new();
let mut buf = [0u8; WASM_PAGE_SIZE_IN_BYTES as usize];
for page_num in 0..memory.size() {
let byte_offset = page_num * u64::try_from(WASM_PAGE_SIZE_IN_BYTES).expect("Amazingly large pages");
memory.read(byte_offset, &mut buf);
hasher.write(&buf);
}
hasher.finish()
}
// Create some toy memory.
let toy_memory = DefaultMemoryImpl::default();
// Populate the memory and hash it.
{
toy_memory.grow(5);
let memory_manager = MemoryManager::init(Rc::clone(&toy_memory));
memory_manager.get(PartitionType::Accounts.memory_id()).grow(1);
memory_manager.get(PartitionType::Accounts.memory_id()).write(0, b"foo");
}
let memory_hash_before = hash_memory(&toy_memory);
// Load the memory into partitions and back again.
let partitions = Partitions::from(Rc::clone(&toy_memory));
let toy_memory_after = partitions.into_memory();
let memory_hash_after = hash_memory(&toy_memory_after);
assert_eq!(
memory_hash_before, memory_hash_after,
"Memory should be unchanged after converting to partitions and back."
);
}

#[derive(Debug, Clone)]
struct GrowingWriteTestVector {
initial_memory_pages: u64,
Expand Down Expand Up @@ -246,7 +212,7 @@ fn growing_write_should_work_for_all() {

#[test]
fn debug_should_portray_partitions_accurately() {
let partitions = Partitions::new(DefaultMemoryImpl::default());
let partitions = Partitions::from(DefaultMemoryImpl::default());
partitions.get(PartitionType::Metadata.memory_id()).grow(5); // Has one page already, storing the schema label. Increase this to 6.
partitions.get(PartitionType::Accounts.memory_id()).grow(2);
assert_eq!(
Expand Down

0 comments on commit 5159856

Please sign in to comment.