perf(history storage): decrease prune_db() loop iteration time by 40-50s on 8gb database
#1173
Conversation
KolbyML
changed the title
prune_db() loop iteration time by 40-50s on 8gb database
trin-history/src/storage.rs
Outdated
| if let Err(err) = self.db_insert(&content_id, &content_key, value) { | ||
| debug!("Error writing content ID {content_id:?} to db: {err:?}"); | ||
| return Err(err); | ||
| } else { | ||
| // we are inserting into the database increase total network storage count |
There was a problem hiding this comment.
| // we are inserting into the database increase total network storage count | |
| // Insertion successful, increase total network storage count |
trin-history/src/storage.rs
Outdated
| @@ -261,6 +267,10 @@ impl HistoryStorage { | |||
| if let Err(err) = self.evict(id_to_remove) { | |||
| debug!("Error writing content ID {id_to_remove:?} to db: {err:?}",); | |||
| } else { | |||
| // we are evicting content from the database decrease total network storage count | |||
There was a problem hiding this comment.
| // we are evicting content from the database decrease total network storage count | |
| // Eviction successful, decrease total network storage count |
trin-history/src/storage.rs
Outdated
| @@ -218,10 +217,15 @@ impl HistoryStorage { | |||
| let content_key: Vec<u8> = key.clone().into(); | |||
| // store content key w/o the 0x prefix | |||
| let content_key = hex_encode(content_key).trim_start_matches("0x").to_string(); | |||
| let value_size = value.len() as u64; | |||
| if let Err(err) = self.db_insert(&content_id, &content_key, value) { | |||
There was a problem hiding this comment.
Just realized that inserting content that already exist will break your measurement - query would succeed, we would increase network_content_storage_used.
I'm not sure about all codepaths and if this is actually an issue, but I think we can be on the safe side if you check for the result of the query and return bool that represents in content was inserted. Fine to leave this for the followup issue or PR.
There was a problem hiding this comment.
We do check upstream if the key is already inserted. But you are right it is free to check the status of the result as well here good catch!.
| @@ -642,11 +641,11 @@ pub mod test { | |||
| let value: Vec<u8> = vec![0; 32000]; | |||
There was a problem hiding this comment.
Can we add a check for network_content_storage_used attribute here? In each iteration, check that it is the expected size. If we can also add a check inside a test somewhere to make sure that it's decremented appropriately, that would be great
trin-history/src/storage.rs
Outdated
| @@ -29,6 +29,7 @@ pub struct HistoryStorage { | |||
| node_id: NodeId, | |||
| node_data_dir: PathBuf, | |||
| storage_capacity_in_bytes: u64, | |||
| network_content_storage_used: u64, | |||
There was a problem hiding this comment.
I don't love this attribute name... Imo, network is somewhat redundant, since we're already inside HistoryStorage & imo that implies that all content that we're dealing with is already from the history network. I also think there's a better word than _used that we can "use" (lol the irony).
There was a problem hiding this comment.
Though, I am struggling thinking of a better alternative.... maybe something like storage_occupied_in_bytes / storage_allocated_in_bytes? Seems like it works well with the storage_capacity_in_bytes
There was a problem hiding this comment.
Maybe... if you agree.... storage_capacity_in_bytes also feels somewhat redundant / probably made more sense when we had a single Storage struct. What about...
storage_capacity_in_bytes -> bytes_capacity
network_content_storage_used -> bytes_occupied
There was a problem hiding this comment.
I like storage_capacity_in_bytes/storage_allocated_in_bytes as it makes it clear what the variable is without relying on knowing what struct you are in.
bytes_occupied of what? having storage_capacity prefix makes it more readable and less ambigious. Expessially for people less familar with the codebase.
What was wrong?
The main performance bottleneck in
prune_db()was callingget_total_storage_usage_in_bytes_from_network()twice per iteration of the loop inprune_db()(once indoes_eviction_cause_under_capacity()and once incapacity_reached()). Which was taking 40-50 seconds in total (2 x 20-25s). We should only be callingget_total_storage_usage_in_bytes_from_network()at the initialization ofHistoryStorageand maintaining the count from their in memory.This 40-50 second result was taken from one of our testnet nodes with a ~8GB sqlite database doing the inital prune_db() at startup initalization.
How was it fixed?
We only insert data or evict data in 2 spots in the code. So we can just keep a count of how much storage we use in memory and increment it or decrement it from there. We will only call
get_total_storage_usage_in_bytes_from_network()once during initialization of the database.