kvdb-rocksdb: expose RocksDB stats #347
Conversation
| let overall_stats = self.stats.overall(); | ||
| let old_cache_hit_count = overall_stats.raw.cache_hit_count; | ||
|
|
||
| self.stats.tally_cache_hit_count(cache_hit_count - old_cache_hit_count); |
There was a problem hiding this comment.
this hack is needed because get_statistics() returns only overall stats, it has probably a race condition
ideas how to make this better are welcome!
| @@ -304,6 +306,8 @@ fn is_corrupted(err: &Error) -> bool { | |||
| fn generate_options(config: &DatabaseConfig) -> Options { | |||
| let mut opts = Options::default(); | |||
|
|
|||
| opts.set_report_bg_io_stats(true); | |||
There was a problem hiding this comment.
well it should be optional, since it is up to 10% performance hit
https://github.com/facebook/rocksdb/wiki/Statistics
There was a problem hiding this comment.
added a config option as requested, this particular parameter (report_bg_io_stats) shouldn't have a perf impact though
There was a problem hiding this comment.
To be clear: background stats has negligable performance impact, while the "other one" can be expensive (10%). Here we're enabling the cheap one for all and making the other configurable. Did I get that right?
ordian
force-pushed
the
ao-rocksdb-stats
branch
from
f6c70d6
to
964efda
Compare
Co-Authored-By: Nikolay Volf <[email protected]>
| @@ -167,6 +167,12 @@ pub struct DatabaseConfig { | |||
| pub columns: u32, | |||
| /// Specify the maximum number of info/debug log files to be kept. | |||
| pub keep_log_file_num: i32, | |||
| /// Enable native RocksDB statistics. | |||
There was a problem hiding this comment.
Maybe a dumb q but can it be enabled/disabled on the fly? Or at start only? Would be fantastic to be able to turn it on dynamically.
There was a problem hiding this comment.
There is only enable_statistics method and no disable_, this option is passed on db::open, so there is no way to change it dynamically unfortunately.
| @@ -304,6 +306,8 @@ fn is_corrupted(err: &Error) -> bool { | |||
| fn generate_options(config: &DatabaseConfig) -> Options { | |||
| let mut opts = Options::default(); | |||
|
|
|||
| opts.set_report_bg_io_stats(true); | |||
There was a problem hiding this comment.
To be clear: background stats has negligable performance impact, while the "other one" can be expensive (10%). Here we're enabling the cheap one for all and making the other configurable. Did I get that right?
| @@ -767,7 +797,7 @@ impl KeyValueDB for Database { | |||
| stats.transactions = taken_stats.raw.transactions; | |||
| stats.bytes_written = taken_stats.raw.bytes_written; | |||
| stats.bytes_read = taken_stats.raw.bytes_read; | |||
|
|
|||
| stats.cache_reads = taken_stats.raw.cache_hit_count; | |||
There was a problem hiding this comment.
I'd keep the same name: stats.cache_hit_count = taken_stats.raw.cache_hit_count;
There was a problem hiding this comment.
that would be a breaking change to kvdb and is out of scope of the PR
* master: kvdb-rocksdb: bump version (#348) kvdb-rocksdb: expose RocksDB stats (#347) Implement Error for FromDecStrErr (#346) Fix clippy lints for rlp-derive (#345) prepare rlp-derive release (#344) Update/change licenses: MIT/Apache2.0 (#342) rlp-derive extracted (#343) Format for readme and changelog corrected (#341) Parity runtime moved to parity common for publication in crates.io (#271) Disable cache if explicit memory budget=0 passed (#339)
For some reason in tests
block.cache.hit/missis always 0, not sure why.