Memory-based pruning history size #4114
Conversation
rphmeier
added
A0-pleasereview 🤓
| // prune all ancient eras until we're below the memory target, | ||
| // but have at least the minimum number of states. | ||
| loop { | ||
| if state.journal_db().mem_used() <= self.config.history_mem { break } |
There was a problem hiding this comment.
slight concern here about the time taken perform this check potentially multiple times per imported block -- an optimization might be done via incremental bookkeeping using the sizes of key-value pairs inserted and deleted into the pruning overlay rather than sweeping the whole overlay each time.
rphmeier
added
A3-inprogress ⏳
|
Hm, apparently |
| @@ -170,7 +171,20 @@ impl SnapshotCommand { | |||
| execute_upgrades(&self.dirs.base, &db_dirs, algorithm, self.compaction.compaction_profile(db_dirs.db_root_path().as_path()))?; | |||
|
|
|||
| // prepare client config | |||
| let client_config = to_client_config(&self.cache_config, Mode::Active, tracing, fat_db, self.compaction, self.wal, VMType::default(), "".into(), algorithm, self.pruning_history, true); | |||
| let client_config = to_client_config( | |||
There was a problem hiding this comment.
the fact that this stuff is duplicated so much is practically a crime :)
Since basically every subcommand needs to initialize a client (and initializing a client needs, at a minimum, some of these parameters), the commands themselves should just take a client config rather than rebuilding it each and every time.
rphmeier
added
A0-pleasereview 🤓
|
Changes Unknown when pulling cc69ab5 on memory-pruning into ** on master**. |
|
This should help with mitigating forks on the testnet: with the default setting of |
| --pruning-memory MB The ideal amount of memory in megabytes to use to store | ||
| recent states. As many states as possible will be kept | ||
| within this limit, and at least --pruning-history states | ||
| will always be kept. (default: {flag_pruning_memory}) |
There was a problem hiding this comment.
2 lines above should be aligned with spaces
arkpar
added
A8-looksgood 🦄
| @@ -578,9 +564,49 @@ impl Client { | |||
| self.db.read().write_buffered(batch); | |||
| chain.commit(); | |||
| self.update_last_hashes(&parent, hash); | |||
|
|
|||
| if let Err(e) = self.prune_ancient(state, &chain) { | |||
There was a problem hiding this comment.
just wondering if this breaks an implicit precondition that the ancient block should be marked canon before the new block is committed.
There was a problem hiding this comment.
This is still analogous to the prior behavior in the case of 1 entry being pruned: journal_under was previously called before mark_canonical. Nothing in the JournalDB interface (or our implementations) should prohibit "runs" of either call without alternation.
|
My main concern about this PR is that it changes the memory usage calculations for the journal DB substantially (they now tend to be 1.5-2x lower than previously) in order to keep them fast enough to call multiple times per-block. |
|
@rphmeier Is that because of additional check here: https://github.com/ethcore/parity/pull/4114/files#diff-698ad0b84c222f83a6207ea3c6d8bc07R170? |
|
@arkpar that check is because values may exist in the overlay multiple times, but we only want to count its size once. the lower calculated memory usage is because we no longer use |
rphmeier
added
the
B7-releasenotes 📜
|
re: memory calculation concerns. I've added a |
Closes #4090
Might also come with changes to informant + a different default setting (currently 150MB)