Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Changes tries build cache #2933

Merged
merged 7 commits into from
Sep 5, 2019
Merged

Changes tries build cache #2933

merged 7 commits into from
Sep 5, 2019

Conversation

svyatonik
Copy link
Contributor

@svyatonik svyatonik commented Jun 24, 2019

Problem: when digest tries are built, we need to read some previous changes tries.
Example: in 4^2 configuration, when block#16 is built, we need to read tries of blocks: 4, 8, 12, 13, 14, 15.
Proposed optimization: maintain in-memory HashMap<Block => Set<ChangedKeys>> && use it instead of reading tries directly from the database.

Some details:

  • if entry for block is missing from the cache, we still read trie from database;
  • the cache is cleared on node shutdown => there could be performance regression on restart;
  • if there's a fork during digest CT creation, then the first imported block purges all used cache entries from the cache => the second block import will read everything from DB. It could be fixed if we would teach cache about finalization/etc, but it'll make it complex + memory demanding.

The cache itself isn't very useful when there's a plenty of memory dedicated to database cache + when top-level digests aren't covering large blocks ranges. E.g. block#512 in 512^1 configuration, that is normally imported in 0.3511s, with this optimization is imported in 0.2932s. Block#1024 in 1024^1 configuration, that is normally imported in 0.6961s, with this optimization is imported in 0.6047s. The difference isn't that big, but if we'll consider situation when there's lack of memory dedicated to database cache (i.e. when tries nodes are actually read from DB, not the rocks-db cache), then the situation seems much better with this optimization: 0.9514s -> 0.6167s.

added CT build cache test
@svyatonik svyatonik force-pushed the changes_trie_build_cache2 branch from a90ecd7 to e9e1f4a Compare July 23, 2019 09:48
@svyatonik svyatonik added A0-please_review Pull request needs code review. and removed A1-onice labels Jul 23, 2019
@gavofyork gavofyork removed the A0-please_review Pull request needs code review. label Aug 8, 2019
@svyatonik svyatonik added A0-please_review Pull request needs code review. and removed A3-needsresolving labels Aug 8, 2019
@svyatonik svyatonik added A3-needsresolving and removed A0-please_review Pull request needs code review. labels Sep 2, 2019
@svyatonik svyatonik added A0-please_review Pull request needs code review. and removed A3-needsresolving labels Sep 2, 2019
@svyatonik svyatonik requested a review from cheme September 2, 2019 14:19
Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache logic looks good.

I could not really assert what happen on fork (doc says it get reinit on first next CT insert, but I could not really find it).

core/client/db/src/lib.rs Outdated Show resolved Hide resolved
core/client/src/in_mem.rs Show resolved Hide resolved
core/state-machine/src/changes_trie/mod.rs Outdated Show resolved Hide resolved
core/state-machine/src/changes_trie/mod.rs Show resolved Hide resolved
core/state-machine/src/changes_trie/mod.rs Outdated Show resolved Hide resolved
core/state-machine/src/changes_trie/build_cache.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@svyatonik svyatonik merged commit ac6a2a7 into master Sep 5, 2019
@svyatonik svyatonik deleted the changes_trie_build_cache2 branch September 5, 2019 05:27
andresilva pushed a commit that referenced this pull request Sep 17, 2019
* changes tries build cache

added CT build cache test

* fix lines width

* fixed some grumbles

* clear cache when: digests disabled, top-level or skewed digest is built

* cached_changed_keys -> with_cached_changed_keys
Demi-Marie pushed a commit to Demi-Marie/substrate that referenced this pull request Sep 17, 2019
* changes tries build cache

added CT build cache test

* fix lines width

* fixed some grumbles

* clear cache when: digests disabled, top-level or skewed digest is built

* cached_changed_keys -> with_cached_changed_keys
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants