Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix/refactor: atomicity and caching #347

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

fix/refactor: atomicity and caching #347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dd43d6f
cleanup
fominok Dec 27, 2024
f7d4105
add version for delta functions
fominok Dec 27, 2024
a771e6a
remove obsolete code
fominok Dec 27, 2024
bcdfbbd
fix misleading comment
fominok Dec 27, 2024
1078307
revert possible cost mismatch for reference insertion
fominok Dec 27, 2024
faeed02
fix compilation issue
fominok Dec 27, 2024
70e0aa9
address some clippy issues
fominok Dec 27, 2024
2749b44
address complex type clippy warning in merk cache
fominok Dec 27, 2024
55acd04
fix doc warning
fominok Dec 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions costs/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,15 @@ impl<T, E> CostResult<T, E> {
pub fn cost_as_result(self) -> Result<OperationCost, E> {
self.value.map(|_| self.cost)
}

/// Call the provided function on success without altering result or cost.
pub fn for_ok(self, f: impl FnOnce(&T)) -> CostResult<T, E> {
if let Ok(x) = &self.value {
f(x)
}

self
}
}

impl<T, E> CostResult<Result<T, E>, E> {
Expand Down Expand Up @@ -170,8 +179,9 @@ impl<T> CostsExt for T {}
/// 1. Early termination on error;
/// 2. Because of 1, `Result` is removed from the equation;
/// 3. `CostContext` is removed too because it is added to external cost
/// accumulator; 4. Early termination uses external cost accumulator so previous
/// costs won't be lost.
/// accumulator;
/// 4. Early termination uses external cost accumulator so previous costs won't
/// be lost.
#[macro_export]
macro_rules! cost_return_on_error {
( &mut $cost:ident, $($body:tt)+ ) => {
Expand All @@ -193,7 +203,7 @@ macro_rules! cost_return_on_error {
/// so no costs will be added except previously accumulated.
#[macro_export]
macro_rules! cost_return_on_error_no_add {
( &$cost:ident, $($body:tt)+ ) => {
( $cost:ident, $($body:tt)+ ) => {
{
use $crate::CostsExt;
let result = { $($body)+ };
Expand Down
10 changes: 5 additions & 5 deletions grovedb-version/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::version::GroveVersion;
use version::GroveVersion;

pub mod error;
pub mod version;
Expand All @@ -8,7 +8,7 @@ macro_rules! check_grovedb_v0_with_cost {
($method:expr, $version:expr) => {{
const EXPECTED_VERSION: u16 = 0;
if $version != EXPECTED_VERSION {
return Err(GroveVersionError::UnknownVersionMismatch {
return Err($crate::error::GroveVersionError::UnknownVersionMismatch {
method: $method.to_string(),
known_versions: vec![EXPECTED_VERSION],
received: $version,
Expand All @@ -24,7 +24,7 @@ macro_rules! check_grovedb_v0 {
($method:expr, $version:expr) => {{
const EXPECTED_VERSION: u16 = 0;
if $version != EXPECTED_VERSION {
return Err(GroveVersionError::UnknownVersionMismatch {
return Err($crate::error::GroveVersionError::UnknownVersionMismatch {
method: $method.to_string(),
known_versions: vec![EXPECTED_VERSION],
received: $version,
Expand All @@ -39,7 +39,7 @@ macro_rules! check_merk_v0_with_cost {
($method:expr, $version:expr) => {{
const EXPECTED_VERSION: u16 = 0;
if $version != EXPECTED_VERSION {
return Err(GroveVersionError::UnknownVersionMismatch {
return Err($crate::error::GroveVersionError::UnknownVersionMismatch {
method: $method.to_string(),
known_versions: vec![EXPECTED_VERSION],
received: $version,
Expand All @@ -55,7 +55,7 @@ macro_rules! check_merk_v0 {
($method:expr, $version:expr) => {{
const EXPECTED_VERSION: u16 = 0;
if $version != EXPECTED_VERSION {
return Err(GroveVersionError::UnknownVersionMismatch {
return Err($crate::error::GroveVersionError::UnknownVersionMismatch {
method: $method.to_string(),
known_versions: vec![EXPECTED_VERSION],
received: $version,
Expand Down
4 changes: 4 additions & 0 deletions grovedb-version/src/version/grovedb_versions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ pub struct GroveDBOperationsGetVersions {
pub get: FeatureVersion,
pub get_caching_optional: FeatureVersion,
pub follow_reference: FeatureVersion,
pub follow_reference_once: FeatureVersion,
pub get_raw: FeatureVersion,
pub get_raw_caching_optional: FeatureVersion,
pub get_raw_optional: FeatureVersion,
Expand Down Expand Up @@ -190,6 +191,7 @@ pub struct GroveDBElementMethodVersions {
pub get_optional_from_storage: FeatureVersion,
pub get_with_absolute_refs: FeatureVersion,
pub get_value_hash: FeatureVersion,
pub get_with_value_hash: FeatureVersion,
pub get_specialized_cost: FeatureVersion,
pub value_defined_cost: FeatureVersion,
pub value_defined_cost_for_serialized_value: FeatureVersion,
Expand All @@ -202,8 +204,10 @@ pub struct GroveDBElementMethodVersions {
pub insert_if_changed_value: FeatureVersion,
pub insert_if_changed_value_into_batch_operations: FeatureVersion,
pub insert_reference: FeatureVersion,
pub insert_reference_if_changed_value: FeatureVersion,
pub insert_reference_into_batch_operations: FeatureVersion,
pub insert_subtree: FeatureVersion,
pub insert_subtree_if_changed: FeatureVersion,
pub insert_subtree_into_batch_operations: FeatureVersion,
pub get_query: FeatureVersion,
pub get_query_values: FeatureVersion,
Expand Down
4 changes: 4 additions & 0 deletions grovedb-version/src/version/v1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ pub const GROVE_V1: GroveVersion = GroveVersion {
get_optional_from_storage: 0,
get_with_absolute_refs: 0,
get_value_hash: 0,
get_with_value_hash: 0,
get_specialized_cost: 0,
value_defined_cost: 0,
value_defined_cost_for_serialized_value: 0,
Expand All @@ -51,8 +52,10 @@ pub const GROVE_V1: GroveVersion = GroveVersion {
insert_if_changed_value: 0,
insert_if_changed_value_into_batch_operations: 0,
insert_reference: 0,
insert_reference_if_changed_value: 0,
insert_reference_into_batch_operations: 0,
insert_subtree: 0,
insert_subtree_if_changed: 0,
insert_subtree_into_batch_operations: 0,
get_query: 0,
get_query_values: 0,
Expand All @@ -71,6 +74,7 @@ pub const GROVE_V1: GroveVersion = GroveVersion {
get: 0,
get_caching_optional: 0,
follow_reference: 0,
follow_reference_once: 0,
get_raw: 0,
get_raw_caching_optional: 0,
get_raw_optional: 0,
Expand Down
8 changes: 4 additions & 4 deletions grovedb/src/batch/estimated_costs/average_case_costs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@
let mut cost = OperationCost::default();

let layer_element_estimates = cost_return_on_error_no_add!(
&cost,
cost,
self.paths.get(path).ok_or_else(|| {
let paths = self
.paths
Expand All @@ -216,9 +216,9 @@
.estimated_to_be_empty();

// Then we have to get the tree
if self.cached_merks.get(path).is_none() {

Check warning on line 219 in grovedb/src/batch/estimated_costs/average_case_costs.rs

View workflow job for this annotation

GitHub Actions / clippy

unnecessary use of `get(path).is_none()` 

warning: unnecessary use of `get(path).is_none()`
   --> grovedb/src/batch/estimated_costs/average_case_costs.rs:219:30
    |
219 |         if self.cached_merks.get(path).is_none() {
    |            ------------------^^^^^^^^^^^^^^^^^^^
    |            |
    |            help: replace it with: `!self.cached_merks.contains_key(path)`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_get_then_check
    = note: `#[warn(clippy::unnecessary_get_then_check)]` on by default
let layer_info = cost_return_on_error_no_add!(
&cost,
cost,
self.paths.get(path).ok_or_else(|| {
let paths = self
.paths
Expand All @@ -233,7 +233,7 @@
})
);
cost_return_on_error_no_add!(
&cost,
cost,
GroveDb::add_average_case_get_merk_at_path::<RocksDbStorage>(
&mut cost,
path,
Expand Down Expand Up @@ -270,22 +270,22 @@
let base_path = KeyInfoPath(vec![]);
if let Some(estimated_layer_info) = self.paths.get(&base_path) {
// Then we have to get the tree
if !self.cached_merks.contains_key(&base_path) {
cost_return_on_error_no_add!(
&cost,
cost,
GroveDb::add_average_case_get_merk_at_path::<RocksDbStorage>(
&mut cost,
&base_path,
estimated_layer_info
.estimated_layer_count
.estimated_to_be_empty(),
estimated_layer_info.is_sum_tree,
grove_version
)
);
self.cached_merks
.insert(base_path, estimated_layer_info.is_sum_tree);
}

Check warning on line 288 in grovedb/src/batch/estimated_costs/average_case_costs.rs

View workflow job for this annotation

GitHub Actions / clippy

usage of `contains_key` followed by `insert` on a `HashMap` 

warning: usage of `contains_key` followed by `insert` on a `HashMap`
   --> grovedb/src/batch/estimated_costs/average_case_costs.rs:273:13
    |
273 | /             if !self.cached_merks.contains_key(&base_path) {
274 | |                 cost_return_on_error_no_add!(
275 | |                     cost,
276 | |                     GroveDb::add_average_case_get_merk_at_path::<RocksDbStorage>(
...   |
287 | |                     .insert(base_path, estimated_layer_info.is_sum_tree);
288 | |             }
    | |_____________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#map_entry
    = note: `#[warn(clippy::map_entry)]` on by default
help: try
    |
273 ~             if let std::collections::hash_map::Entry::Vacant(e) = self.cached_merks.entry(base_path) {
274 +                 cost_return_on_error_no_add!(
275 +                     cost,
276 +                     GroveDb::add_average_case_get_merk_at_path::<RocksDbStorage>(
277 +                         &mut cost,
278 +                         &base_path,
279 +                         estimated_layer_info
280 +                             .estimated_layer_count
281 +                             .estimated_to_be_empty(),
282 +                         estimated_layer_info.is_sum_tree,
283 +                         grove_version
284 +                     )
285 +                 );
286 +                 e.insert(estimated_layer_info.is_sum_tree);
287 +             }
    |
}
Ok(()).wrap_with_cost(cost)
}
Expand Down
6 changes: 3 additions & 3 deletions grovedb/src/batch/estimated_costs/worst_case_costs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ impl<G, SR> TreeCache<G, SR> for WorstCaseTreeCacheKnownPaths {
let mut cost = OperationCost::default();

let worst_case_layer_element_estimates = cost_return_on_error_no_add!(
&cost,
cost,
self.paths
.get(path)
.ok_or_else(|| Error::PathNotFoundInCacheForEstimatedCosts(format!(
Expand All @@ -204,7 +204,7 @@ impl<G, SR> TreeCache<G, SR> for WorstCaseTreeCacheKnownPaths {
// Then we have to get the tree
if !self.cached_merks.contains(path) {
cost_return_on_error_no_add!(
&cost,
cost,
GroveDb::add_worst_case_get_merk_at_path::<RocksDbStorage>(
&mut cost,
path,
Expand Down Expand Up @@ -247,7 +247,7 @@ impl<G, SR> TreeCache<G, SR> for WorstCaseTreeCacheKnownPaths {
// Then we have to get the tree
if !self.cached_merks.contains(&base_path) {
cost_return_on_error_no_add!(
&cost,
cost,
GroveDb::add_worst_case_get_merk_at_path::<RocksDbStorage>(
&mut cost,
&base_path,
Expand Down
14 changes: 6 additions & 8 deletions grovedb/src/batch/just_in_time_reference_update.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,17 @@
F: FnMut(&[Vec<u8>], bool) -> CostResult<Merk<S>, Error>,
S: StorageContext<'db>,
{
pub(crate) fn process_old_element_flags<G, SR>(
key: &[u8],
serialized: &[u8],
new_element: &mut Element,
old_element: Element,
old_serialized_element: &[u8],
is_in_sum_tree: bool,
flags_update: &mut G,
split_removal_bytes: &mut SR,
grove_version: &GroveVersion,
) -> CostResult<CryptoHash, Error>

Check warning on line 38 in grovedb/src/batch/just_in_time_reference_update.rs

View workflow job for this annotation

GitHub Actions / clippy

this function has too many arguments (9/7) 

warning: this function has too many arguments (9/7)
  --> grovedb/src/batch/just_in_time_reference_update.rs:28:5
   |
28 | /     pub(crate) fn process_old_element_flags<G, SR>(
29 | |         key: &[u8],
30 | |         serialized: &[u8],
31 | |         new_element: &mut Element,
...  |
37 | |         grove_version: &GroveVersion,
38 | |     ) -> CostResult<CryptoHash, Error>
   | |______________________________________^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
   = note: `#[warn(clippy::too_many_arguments)]` on by default
where
G: FnMut(&StorageCost, Option<ElementFlags>, &mut ElementFlags) -> Result<bool, Error>,
SR: FnMut(
Expand All @@ -53,13 +53,13 @@
updated_new_element_with_old_flags.set_flags(maybe_old_flags.clone());
// There are no storage flags, we can just hash new element
let new_serialized_bytes = cost_return_on_error_no_add!(
&cost,
cost,
updated_new_element_with_old_flags.serialize(grove_version)
);
let val_hash = value_hash(&new_serialized_bytes).unwrap_add_cost(&mut cost);
Ok(val_hash).wrap_with_cost(cost)
} else {
let val_hash = value_hash(&serialized).unwrap_add_cost(&mut cost);

Check warning on line 62 in grovedb/src/batch/just_in_time_reference_update.rs

View workflow job for this annotation

GitHub Actions / clippy

this expression creates a reference which is immediately dereferenced by the compiler 

warning: this expression creates a reference which is immediately dereferenced by the compiler
  --> grovedb/src/batch/just_in_time_reference_update.rs:62:47
   |
62 |                     let val_hash = value_hash(&serialized).unwrap_add_cost(&mut cost);
   |                                               ^^^^^^^^^^^ help: change this to: `serialized`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
   = note: `#[warn(clippy::needless_borrow)]` on by default
Ok(val_hash).wrap_with_cost(cost)
}
} else {
Expand Down Expand Up @@ -93,7 +93,7 @@
updated_new_element_with_old_flags.set_flags(maybe_old_flags.clone());

let serialized_with_old_flags = cost_return_on_error_no_add!(
&cost,
cost,
updated_new_element_with_old_flags.serialize(grove_version)
);
KV::node_value_byte_cost_size(
Expand All @@ -115,7 +115,7 @@
if let Some(old_element_flags) = maybe_old_flags.as_mut() {
if let BasicStorageRemoval(removed_bytes) = storage_costs.removed_bytes {
let (_, value_removed_bytes) = cost_return_on_error_no_add!(
&cost,
cost,
split_removal_bytes(old_element_flags, 0, removed_bytes)
);
storage_costs.removed_bytes = value_removed_bytes;
Expand All @@ -125,7 +125,7 @@
let mut new_element_cloned = original_new_element.clone();

let changed = cost_return_on_error_no_add!(
&cost,
cost,
(flags_update)(
&storage_costs,
maybe_old_flags.clone(),
Expand All @@ -145,10 +145,8 @@
return Ok(val_hash).wrap_with_cost(cost);
} else {
// There are no storage flags, we can just hash new element
let new_serialized_bytes = cost_return_on_error_no_add!(
&cost,
new_element_cloned.serialize(grove_version)
);
let new_serialized_bytes =
cost_return_on_error_no_add!(cost, new_element_cloned.serialize(grove_version));

new_storage_cost = KV::node_value_byte_cost_size(
key.len() as u32,
Expand Down
Loading
Loading