From b18d6087944de77e6d9db998006c883687ba416b Mon Sep 17 00:00:00 2001 From: austinabell Date: Sat, 30 Jan 2021 18:52:02 -0500 Subject: [PATCH 1/6] Switch cache to OnceCell --- Cargo.lock | 2 +- ipld/amt/Cargo.toml | 2 +- ipld/amt/src/node.rs | 38 ++++++++++++++++---------------------- 3 files changed, 18 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bae2cc16ab09..981dbc024392 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3323,7 +3323,7 @@ dependencies = [ "forest_db", "forest_encoding", "ipld_blockstore", - "lazycell", + "once_cell", "serde", "thiserror", ] diff --git a/ipld/amt/Cargo.toml b/ipld/amt/Cargo.toml index 42bae48d9289..6a2fd7046f94 100644 --- a/ipld/amt/Cargo.toml +++ b/ipld/amt/Cargo.toml @@ -15,7 +15,7 @@ encoding = { package = "forest_encoding", version = "0.2.1" } serde = { version = "1.0", features = ["derive"] } ipld_blockstore = "0.1" thiserror = "1.0" -lazycell = "1.2.1" +once_cell = "1.5" ahash = { version = "0.6", optional = true } [features] diff --git a/ipld/amt/src/node.rs b/ipld/amt/src/node.rs index b37d509b52b5..88e9b51b569c 100644 --- a/ipld/amt/src/node.rs +++ b/ipld/amt/src/node.rs @@ -8,7 +8,7 @@ use encoding::{ ser::{self, Serialize}, }; use ipld_blockstore::BlockStore; -use lazycell::LazyCell; +use once_cell::unsync::OnceCell; use std::error::Error as StdError; use super::ValueMut; @@ -19,7 +19,7 @@ pub(super) enum Link { /// Unchanged link to data with an atomic cache. Cid { cid: Cid, - cache: LazyCell>>, + cache: OnceCell>>, }, /// Modifications have been made to the link, requires flush to clear Dirty(Box>), @@ -75,14 +75,8 @@ impl Default for Node { } /// Turns the WIDTH length array into a vector for serialization -fn values_to_vec(bmap: BitMap, values: &[Option; WIDTH]) -> Vec<&T> { - let mut v: Vec<&T> = Vec::new(); - for (i, _) in values.iter().enumerate().take(WIDTH) { - if bmap.get_bit(i as u64) { - v.push(values[i].as_ref().unwrap()) - } - } - v +fn values_to_vec(values: &[Option]) -> Vec<&T> { + values.iter().filter_map(|val| val.as_ref()).collect() } /// Puts values from vector into shard array @@ -125,7 +119,9 @@ where S: ser::Serializer, { match &self { - Node::Leaf { bmap, vals } => (bmap, [0u8; 0], values_to_vec(*bmap, &vals)).serialize(s), + Node::Leaf { bmap, vals } => { + (bmap, [0u8; 0], values_to_vec(vals.as_ref())).serialize(s) + } Node::Link { bmap, links } => { let cids = cids_from_links(links).map_err(|e| ser::Error::custom(e.to_string()))?; (bmap, cids, [0u8; 0]).serialize(s) @@ -173,7 +169,7 @@ where if let Some(Link::Cid { cache, .. }) = link { // Yes, this is necessary to interop, and yes this is safe to borrow // mutably because there are no values changed here, just extra db writes. - if let Some(cached) = cache.borrow_mut() { + if let Some(cached) = cache.get_mut() { cached.flush(bs)?; bs.put(cached, Blake2b256)?; } @@ -186,11 +182,9 @@ where // Puts node in blockstore and and retrieves it's CID let cid = bs.put(n, Blake2b256)?; - let cache = LazyCell::new(); - // Can keep the flushed node in link cache let node = std::mem::take(n); - let _ = cache.fill(node); + let cache = OnceCell::from(node); // Turn dirty node into a Cid link *link = Some(Link::Cid { cid, cache }); @@ -230,7 +224,7 @@ where Node::Leaf { vals, .. } => Ok(vals[i as usize].as_ref()), Node::Link { links, .. } => match &links[sub_i as usize] { Some(Link::Cid { cid, cache }) => { - if let Some(cached_node) = cache.borrow() { + if let Some(cached_node) = cache.get() { cached_node.get(bs, height - 1, i % nodes_for_height(height)) } else { let node: Box> = bs @@ -238,8 +232,8 @@ where .ok_or_else(|| Error::CidNotFound(cid.to_string()))?; // Ignore error intentionally, the cache value will always be the same - let _ = cache.fill(node); - let cache_node = cache.borrow().expect("cache filled on line above"); + let _ = cache.set(node); + let cache_node = cache.get().expect("cache filled on line above"); cache_node.get(bs, height - 1, i % nodes_for_height(height)) } } @@ -387,7 +381,7 @@ where // Replace cache, no node deleted. // Error can be ignored because value will always be the same // even if possible to hit race condition. - let _ = cache.fill(sub_node); + let _ = cache.set(sub_node); // Index to be deleted was not found return Ok(false); @@ -446,7 +440,7 @@ where let keep_going = match l.as_ref().expect("bit set at index") { Link::Dirty(sub) => sub.for_each_while(store, height - 1, offs, f)?, Link::Cid { cid, cache } => { - if let Some(cached_node) = cache.borrow() { + if let Some(cached_node) = cache.get() { cached_node.for_each_while(store, height - 1, offs, f)? } else { let node: Box> = store @@ -542,14 +536,14 @@ where #[cfg(feature = "go-interop")] { if cached { - let _ = cache.fill(node); + let _ = cache.set(node); } } // Replace cache, or else iteration over without modification // will consume cache #[cfg(not(feature = "go-interop"))] - let _ = cache.fill(node); + let _ = cache.set(node); } (keep_going, did_mutate_node) From 428d832764b4983a281a7c2f557f6ee35a360bc4 Mon Sep 17 00:00:00 2001 From: austinabell Date: Sat, 30 Jan 2021 19:03:39 -0500 Subject: [PATCH 2/6] Remove lazycell from hamt --- Cargo.lock | 2 +- ipld/amt/src/node.rs | 5 +---- ipld/hamt/Cargo.toml | 2 +- ipld/hamt/src/node.rs | 16 +++++++--------- ipld/hamt/src/pointer.rs | 4 ++-- 5 files changed, 12 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 981dbc024392..2d2484ccd9c1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3353,7 +3353,7 @@ dependencies = [ "forest_ipld", "hex", "ipld_blockstore", - "lazycell", + "once_cell", "serde", "sha2 0.9.2", "thiserror", diff --git a/ipld/amt/src/node.rs b/ipld/amt/src/node.rs index 88e9b51b569c..22de83f038f0 100644 --- a/ipld/amt/src/node.rs +++ b/ipld/amt/src/node.rs @@ -450,10 +450,7 @@ where #[cfg(not(feature = "go-interop"))] { // Ignore error intentionally, the cache value will always be the same - let _ = cache.fill(node); - let cache_node = - cache.borrow().expect("cache filled on line above"); - + let cache_node = cache.get_or_init(|| node); cache_node.for_each_while(store, height - 1, offs, f)? } diff --git a/ipld/hamt/Cargo.toml b/ipld/hamt/Cargo.toml index 54e233e61320..528c252ed31a 100644 --- a/ipld/hamt/Cargo.toml +++ b/ipld/hamt/Cargo.toml @@ -19,7 +19,7 @@ forest_ipld = "0.1.1" serde_bytes = { package = "cs_serde_bytes", version = "0.12" } thiserror = "1.0" sha2 = "0.9.1" -lazycell = "1.2.1" +once_cell = "1.5" forest_hash_utils = "0.1" [features] diff --git a/ipld/hamt/src/node.rs b/ipld/hamt/src/node.rs index 990d7d7231ff..e4c4d84a526f 100644 --- a/ipld/hamt/src/node.rs +++ b/ipld/hamt/src/node.rs @@ -7,7 +7,7 @@ use super::pointer::Pointer; use super::{Error, Hash, HashAlgorithm, KeyValuePair, MAX_ARRAY_WIDTH}; use cid::Code::Blake2b256; use ipld_blockstore::BlockStore; -use lazycell::LazyCell; +use once_cell::unsync::OnceCell; use serde::de::DeserializeOwned; use serde::{Deserialize, Deserializer, Serialize, Serializer}; use std::borrow::Borrow; @@ -129,7 +129,7 @@ where for p in &self.pointers { match p { Pointer::Link { cid, cache } => { - if let Some(cached_node) = cache.borrow() { + if let Some(cached_node) = cache.get() { cached_node.for_each(store, f)? } else { let node = if let Some(node) = store.get(cid)? { @@ -143,8 +143,7 @@ where }; // Ignore error intentionally, the cache value will always be the same - let _ = cache.fill(node); - let cache_node = cache.borrow().expect("cache filled on line above"); + let cache_node = cache.get_or_init(|| node); cache_node.for_each(store, f)? } } @@ -196,7 +195,7 @@ where let child = self.get_child(cindex); match child { Pointer::Link { cid, cache } => { - if let Some(cached_node) = cache.borrow() { + if let Some(cached_node) = cache.get() { // Link node is cached cached_node.get_value(hashed_key, bit_width, depth + 1, key, store) } else { @@ -211,8 +210,7 @@ where }; // Intentionally ignoring error, cache will always be the same. - let _ = cache.fill(node); - let cache_node = cache.borrow().expect("cache filled on line above"); + let cache_node = cache.get_or_init(|| node); cache_node.get_value(hashed_key, bit_width, depth + 1, key, store) } } @@ -393,13 +391,13 @@ where // Put node in blockstore and retrieve Cid let cid = store.put(node, Blake2b256)?; - let cache = LazyCell::new(); + let cache = OnceCell::new(); #[cfg(not(feature = "go-interop"))] { // Can keep the flushed node in link cache let node = std::mem::take(node); - let _ = cache.fill(node); + let _ = cache.set(node); } // Replace cached node with Cid link diff --git a/ipld/hamt/src/pointer.rs b/ipld/hamt/src/pointer.rs index aa5e11b63075..b707da5f7444 100644 --- a/ipld/hamt/src/pointer.rs +++ b/ipld/hamt/src/pointer.rs @@ -4,7 +4,7 @@ use super::node::Node; use super::{Error, Hash, HashAlgorithm, KeyValuePair, MAX_ARRAY_WIDTH}; use cid::Cid; -use lazycell::LazyCell; +use once_cell::unsync::OnceCell; use serde::de::{self, DeserializeOwned}; use serde::ser; use serde::{Deserialize, Deserializer, Serialize, Serializer}; @@ -15,7 +15,7 @@ pub(crate) enum Pointer { Values(Vec>), Link { cid: Cid, - cache: LazyCell>>, + cache: OnceCell>>, }, Dirty(Box>), } From 2687d3158a5bf463f519b6906ee22793759d2ea6 Mon Sep 17 00:00:00 2001 From: austinabell Date: Sat, 30 Jan 2021 19:09:26 -0500 Subject: [PATCH 3/6] Replace state manager lazycell --- Cargo.lock | 2 +- blockchain/state_manager/Cargo.toml | 2 +- blockchain/state_manager/src/lib.rs | 10 ++--- .../state_manager/src/vm_circ_supply.rs | 41 +++++++++---------- 4 files changed, 26 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2d2484ccd9c1..95198ce1085c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6216,10 +6216,10 @@ dependencies = [ "ipld_amt 0.2.0", "ipld_blockstore", "lazy_static", - "lazycell", "log", "networks", "num-traits 0.2.14", + "once_cell", "serde", "state_tree", "statediff", diff --git a/blockchain/state_manager/Cargo.toml b/blockchain/state_manager/Cargo.toml index c9b5428c192b..5fee7634b539 100644 --- a/blockchain/state_manager/Cargo.toml +++ b/blockchain/state_manager/Cargo.toml @@ -41,7 +41,7 @@ filecoin-proofs-api = { version = "5.4.1", features = [ futures = "0.3.5" runtime = { package = "forest_runtime", version = "0.1" } lazy_static = "1.4" -lazycell = "1.2.1" +once_cell = "1.5" forest_crypto = { version = "0.4" } networks = { path = "../../types/networks" } statediff = { path = "../../utils/statediff", optional = true } diff --git a/blockchain/state_manager/src/lib.rs b/blockchain/state_manager/src/lib.rs index 576e373b60ff..55f2ec568390 100644 --- a/blockchain/state_manager/src/lib.rs +++ b/blockchain/state_manager/src/lib.rs @@ -36,13 +36,13 @@ use futures::FutureExt; use interpreter::{resolve_to_key_addr, ApplyRet, BlockMessages, Rand, VM}; use interpreter::{CircSupplyCalc, LookbackStateGetter}; use ipld_amt::Amt; -use lazycell::AtomicLazyCell; use log::{debug, info, trace, warn}; use message::{message_receipt, unsigned_message}; use message::{ChainMessage, Message, MessageReceipt, UnsignedMessage}; use networks::get_network_version_default; use num_bigint::{bigint_ser, BigInt}; use num_traits::identities::Zero; +use once_cell::sync::OnceCell; use serde::{Deserialize, Serialize}; use state_tree::StateTree; use std::collections::HashMap; @@ -473,14 +473,14 @@ where { // This isn't ideal to have, since the execution is syncronous, but this needs to be the // case because the state transition has to be in blocking thread to avoid starving executor - let outm: AtomicLazyCell = Default::default(); - let outr: AtomicLazyCell = Default::default(); + let outm: OnceCell = Default::default(); + let outr: OnceCell = Default::default(); let m_clone = outm.clone(); let r_clone = outr.clone(); let callback = move |cid: &Cid, unsigned: &ChainMessage, apply_ret: &ApplyRet| { if *cid == mcid { - let _ = m_clone.fill(unsigned.message().clone()); - let _ = r_clone.fill(apply_ret.clone()); + let _ = m_clone.set(unsigned.message().clone()); + let _ = r_clone.set(apply_ret.clone()); return Err("halt".to_string()); } diff --git a/blockchain/state_manager/src/vm_circ_supply.rs b/blockchain/state_manager/src/vm_circ_supply.rs index 7ce2ef7a95c4..e52556040eee 100644 --- a/blockchain/state_manager/src/vm_circ_supply.rs +++ b/blockchain/state_manager/src/vm_circ_supply.rs @@ -10,12 +10,12 @@ use cid::Cid; use clock::ChainEpoch; use fil_types::{FILECOIN_PRECISION, FIL_RESERVED}; use interpreter::CircSupplyCalc; -use lazycell::AtomicLazyCell; use networks::{ UPGRADE_ACTORS_V2_HEIGHT, UPGRADE_CALICO_HEIGHT, UPGRADE_IGNITION_HEIGHT, UPGRADE_LIFTOFF_HEIGHT, }; use num_bigint::BigInt; +use once_cell::sync::OnceCell; use state_tree::StateTree; use std::error::Error as StdError; use vm::{ActorState, TokenAmount}; @@ -51,8 +51,8 @@ pub struct GenesisInfo { vesting: GenesisInfoVesting, /// info about the Accounts in the genesis state - pub genesis_pledge: AtomicLazyCell, - pub genesis_market_funds: AtomicLazyCell, + pub genesis_pledge: OnceCell, + pub genesis_market_funds: OnceCell, } impl GenesisInfo { @@ -67,8 +67,8 @@ impl GenesisInfo { let _ = self .genesis_market_funds - .fill(get_fil_market_locked(&state_tree)?); - let _ = self.genesis_pledge.fill(get_fil_power_locked(&state_tree)?); + .set(get_fil_market_locked(&state_tree)?); + let _ = self.genesis_pledge.set(get_fil_power_locked(&state_tree)?); Ok(()) } @@ -76,9 +76,9 @@ impl GenesisInfo { #[derive(Default)] pub struct GenesisInfoVesting { - pub genesis: AtomicLazyCell>, - pub ignition: AtomicLazyCell>, - pub calico: AtomicLazyCell>, + pub genesis: OnceCell>, + pub ignition: OnceCell>, + pub calico: OnceCell>, } impl CircSupplyCalc for GenesisInfo { @@ -92,18 +92,15 @@ impl CircSupplyCalc for GenesisInfo { // but it's not ideal to have the side effect from the VM to modify the genesis info // of the state manager. This isn't terrible because it's just caching to avoid // recalculating using the store, and it avoids computing until circ_supply is called. - if !self.vesting.genesis.filled() { + if !self.vesting.genesis.get().is_some() { self.init(state_tree.store())?; - let _ = self.vesting.genesis.fill(setup_genesis_vesting_schedule()); + let _ = self.vesting.genesis.set(setup_genesis_vesting_schedule()); } - if !self.vesting.ignition.filled() { - let _ = self - .vesting - .ignition - .fill(setup_ignition_vesting_schedule()); + if !self.vesting.ignition.get().is_some() { + let _ = self.vesting.ignition.set(setup_ignition_vesting_schedule()); } - if !self.vesting.calico.filled() { - let _ = self.vesting.calico.fill(setup_calico_vesting_schedule()); + if !self.vesting.calico.get().is_some() { + let _ = self.vesting.calico.set(setup_calico_vesting_schedule()); } get_circulating_supply(&self, height, state_tree) @@ -128,17 +125,17 @@ pub fn get_fil_vested( let pre_ignition = genesis_info .vesting .genesis - .borrow() + .get() .expect("Pre ignition should be initialized"); let post_ignition = genesis_info .vesting .ignition - .borrow() + .get() .expect("Post ignition should be initialized"); let calico_vesting = genesis_info .vesting .calico - .borrow() + .get() .expect("calico vesting should be initialized"); if height <= UPGRADE_IGNITION_HEIGHT { @@ -160,11 +157,11 @@ pub fn get_fil_vested( if height <= UPGRADE_ACTORS_V2_HEIGHT { return_value += genesis_info .genesis_pledge - .borrow() + .get() .expect("Genesis info should be initialized") + genesis_info .genesis_market_funds - .borrow() + .get() .expect("Genesis info should be initialized"); } From d7e7bec81ae9777468bc3c5353fd067951a5d0ed Mon Sep 17 00:00:00 2001 From: austinabell Date: Sun, 31 Jan 2021 09:20:46 -0500 Subject: [PATCH 4/6] Cleanup init in amt --- ipld/amt/src/node.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/ipld/amt/src/node.rs b/ipld/amt/src/node.rs index 22de83f038f0..2a1f410ad0f2 100644 --- a/ipld/amt/src/node.rs +++ b/ipld/amt/src/node.rs @@ -224,18 +224,13 @@ where Node::Leaf { vals, .. } => Ok(vals[i as usize].as_ref()), Node::Link { links, .. } => match &links[sub_i as usize] { Some(Link::Cid { cid, cache }) => { - if let Some(cached_node) = cache.get() { - cached_node.get(bs, height - 1, i % nodes_for_height(height)) - } else { - let node: Box> = bs - .get(cid)? - .ok_or_else(|| Error::CidNotFound(cid.to_string()))?; - - // Ignore error intentionally, the cache value will always be the same - let _ = cache.set(node); - let cache_node = cache.get().expect("cache filled on line above"); - cache_node.get(bs, height - 1, i % nodes_for_height(height)) - } + let cached_node = + cache.get_or_try_init(|| -> Result>, Error> { + bs.get(cid)? + .ok_or_else(|| Error::CidNotFound(cid.to_string())) + })?; + + cached_node.get(bs, height - 1, i % nodes_for_height(height)) } Some(Link::Dirty(n)) => n.get(bs, height - 1, i % nodes_for_height(height)), None => Ok(None), @@ -440,6 +435,7 @@ where let keep_going = match l.as_ref().expect("bit set at index") { Link::Dirty(sub) => sub.for_each_while(store, height - 1, offs, f)?, Link::Cid { cid, cache } => { + // TODO simplify with try_init when go-interop feature not needed if let Some(cached_node) = cache.get() { cached_node.for_each_while(store, height - 1, offs, f)? } else { From 77252a72153aa860484018f375dcd6abb40d16f5 Mon Sep 17 00:00:00 2001 From: austinabell Date: Sun, 31 Jan 2021 09:24:58 -0500 Subject: [PATCH 5/6] cleanup vesting initialization --- .../state_manager/src/vm_circ_supply.rs | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/blockchain/state_manager/src/vm_circ_supply.rs b/blockchain/state_manager/src/vm_circ_supply.rs index e52556040eee..a8c880df408a 100644 --- a/blockchain/state_manager/src/vm_circ_supply.rs +++ b/blockchain/state_manager/src/vm_circ_supply.rs @@ -92,16 +92,20 @@ impl CircSupplyCalc for GenesisInfo { // but it's not ideal to have the side effect from the VM to modify the genesis info // of the state manager. This isn't terrible because it's just caching to avoid // recalculating using the store, and it avoids computing until circ_supply is called. - if !self.vesting.genesis.get().is_some() { - self.init(state_tree.store())?; - let _ = self.vesting.genesis.set(setup_genesis_vesting_schedule()); - } - if !self.vesting.ignition.get().is_some() { - let _ = self.vesting.ignition.set(setup_ignition_vesting_schedule()); - } - if !self.vesting.calico.get().is_some() { - let _ = self.vesting.calico.set(setup_calico_vesting_schedule()); - } + self.vesting + .genesis + .get_or_try_init(|| -> Result<_, Box> { + self.init(state_tree.store())?; + Ok(setup_genesis_vesting_schedule()) + })?; + + self.vesting + .ignition + .get_or_init(|| setup_ignition_vesting_schedule()); + + self.vesting + .calico + .get_or_init(|| setup_calico_vesting_schedule()); get_circulating_supply(&self, height, state_tree) } From 4391ad771ffcb2ab3011ecba62a18a7bc35be43f Mon Sep 17 00:00:00 2001 From: austinabell Date: Mon, 1 Feb 2021 08:18:05 -0500 Subject: [PATCH 6/6] lint --- blockchain/state_manager/src/vm_circ_supply.rs | 4 ++-- utils/paramfetch/src/lib.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/blockchain/state_manager/src/vm_circ_supply.rs b/blockchain/state_manager/src/vm_circ_supply.rs index a8c880df408a..0a3e1d8a8c75 100644 --- a/blockchain/state_manager/src/vm_circ_supply.rs +++ b/blockchain/state_manager/src/vm_circ_supply.rs @@ -101,11 +101,11 @@ impl CircSupplyCalc for GenesisInfo { self.vesting .ignition - .get_or_init(|| setup_ignition_vesting_schedule()); + .get_or_init(setup_ignition_vesting_schedule); self.vesting .calico - .get_or_init(|| setup_calico_vesting_schedule()); + .get_or_init(setup_calico_vesting_schedule); get_circulating_supply(&self, height, state_tree) } diff --git a/utils/paramfetch/src/lib.rs b/utils/paramfetch/src/lib.rs index 47b263663178..ec345b18cb18 100644 --- a/utils/paramfetch/src/lib.rs +++ b/utils/paramfetch/src/lib.rs @@ -137,7 +137,7 @@ async fn fetch_verify_params( Ok(()) => return Ok(()), Err(e) => { if e.kind() != ErrorKind::NotFound { - warn!("{}", e) + warn!("Error checking file: {}", e); } } }