-
Notifications
You must be signed in to change notification settings - Fork 1.5k
WIP: Add database read/write/delete timing metrics #2839
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.
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
base: staging
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,7 +100,16 @@ impl< | |
| } | ||
| // Otherwise, insert the key-value pair directly into the map. | ||
| false => { | ||
| self.map.write().insert(bincode::serialize(&key)?, value); | ||
| let serialized_key = bincode::serialize(&key)?; | ||
| let start = std::time::Instant::now(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, but those values are likely to be miniscule when running in memory only |
||
| self.map.write().insert(serialized_key, value); | ||
| let duration = start.elapsed().as_secs_f64(); | ||
| snarkvm_metrics::histogram_label( | ||
| snarkvm_metrics::database::WRITE_DURATION, | ||
| "map_type", | ||
| "memory".to_string(), | ||
| duration, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -119,7 +128,16 @@ impl< | |
| } | ||
| // Otherwise, remove the key-value pair directly from the map. | ||
| false => { | ||
| self.map.write().remove(&bincode::serialize(&key)?); | ||
| let serialized_key = bincode::serialize(&key)?; | ||
| let start = std::time::Instant::now(); | ||
| self.map.write().remove(&serialized_key); | ||
| let duration = start.elapsed().as_secs_f64(); | ||
| snarkvm_metrics::histogram_label( | ||
| snarkvm_metrics::database::DELETE_DURATION, | ||
| "map_type", | ||
| "memory".to_string(), | ||
| duration, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -217,12 +235,24 @@ impl< | |
| .collect::<Result<Vec<_>>>()?; | ||
|
|
||
| // Perform all the queued operations. | ||
| let start = std::time::Instant::now(); | ||
| for (key, value) in prepared_operations { | ||
| match value { | ||
| Some(value) => locked_map.insert(key, value), | ||
| None => locked_map.remove(&key), | ||
| Some(value) => { | ||
| locked_map.insert(key, value); | ||
| } | ||
| None => { | ||
| locked_map.remove(&key); | ||
| } | ||
| }; | ||
| } | ||
| let duration = start.elapsed().as_secs_f64(); | ||
| snarkvm_metrics::histogram_label( | ||
| snarkvm_metrics::database::WRITE_DURATION, | ||
| "map_type", | ||
| "memory_batch".to_string(), | ||
| duration, | ||
| ); | ||
| } | ||
|
|
||
| // Clear the checkpoint stack. | ||
|
|
@@ -315,7 +345,17 @@ impl< | |
| K: Borrow<Q>, | ||
| Q: PartialEq + Eq + Hash + Serialize + ?Sized, | ||
| { | ||
| Ok(self.map.read().get(&bincode::serialize(key)?).cloned().map(Cow::Owned)) | ||
| let serialized_key = bincode::serialize(key)?; | ||
| let start = std::time::Instant::now(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: with this many uses I'd import the |
||
| let result = self.map.read().get(&serialized_key).cloned(); | ||
| let duration = start.elapsed().as_secs_f64(); | ||
| snarkvm_metrics::histogram_label( | ||
| snarkvm_metrics::database::READ_DURATION, | ||
| "map_type", | ||
| "memory".to_string(), | ||
| duration, | ||
| ); | ||
| Ok(result.map(Cow::Owned)) | ||
| } | ||
|
|
||
| /// | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,16 +16,26 @@ | |
| #![forbid(unsafe_code)] | ||
|
|
||
| const GAUGE_NAMES: [&str; 1] = [committee::TOTAL_STAKE]; | ||
| const HISTOGRAM_NAMES: [&str; 3] = [database::READ_DURATION, database::WRITE_DURATION, database::DELETE_DURATION]; | ||
|
|
||
| pub mod committee { | ||
| pub const TOTAL_STAKE: &str = "snarkvm_ledger_committee_total_stake"; | ||
| } | ||
|
|
||
| pub mod database { | ||
| pub const READ_DURATION: &str = "snarkvm_database_read_duration_seconds"; | ||
| pub const WRITE_DURATION: &str = "snarkvm_database_write_duration_seconds"; | ||
| pub const DELETE_DURATION: &str = "snarkvm_database_delete_duration_seconds"; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are the numbers for deletion any different for the "plain" writes? we might just need |
||
| } | ||
|
|
||
| /// Registers all snarkVM metrics. | ||
| pub fn register_metrics() { | ||
| for name in GAUGE_NAMES { | ||
| register_gauge(name); | ||
| } | ||
| for name in HISTOGRAM_NAMES { | ||
| register_histogram(name); | ||
| } | ||
| } | ||
|
|
||
| /******** Counter ********/ | ||
|
|
@@ -103,3 +113,15 @@ pub fn histogram<V: Into<f64>>(name: &'static str, value: V) { | |
| pub fn histogram_label<V: Into<f64>>(name: &'static str, label_key: &'static str, label_value: String, value: V) { | ||
| ::metrics::histogram!(name, label_key => label_value).record(value.into()); | ||
| } | ||
|
|
||
| /// Times a database operation and records the duration with the given map type label. | ||
| pub fn time_database_operation<F, R>(metric_name: &'static str, map_type: &str, operation: F) -> R | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doesn't seem like it's used at the moment, but this (or a macro) would be preferable for readability and performance (feature-gating) |
||
| where | ||
| F: FnOnce() -> R, | ||
| { | ||
| let start = std::time::Instant::now(); | ||
| let result = operation(); | ||
| let duration = start.elapsed().as_secs_f64(); | ||
| histogram_label(metric_name, "map_type", map_type.to_string(), duration); | ||
| result | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we feature-gate it (
metrics)? obtaining the time is a perf penalty in itselfThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would work best with a simple macro taking a block containing the section to be measured