Skip to content
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

Introduce prefixed storage with enumeration #4185

Merged
merged 19 commits into from Dec 9, 2019
Merged

Introduce prefixed storage with enumeration #4185

merged 19 commits into from Dec 9, 2019

Conversation

@thiolliere
Copy link
Contributor

thiolliere commented Nov 22, 2019

DONE:

  • a new trait StoragePrefixedMap is introduced and implemened for map, linked_map, double_map. it allows for enumeration and removal.
  • ChangeSetOverlay is now using BTreeMap instead of HashMap.
  • InMemory backend is now using BTreeMap instead of HashMap.
  • sr-io has a new function next_key(key)-> Option<key> returning the next key in the storage.

TODO:

  • cache the backend next_key in Ext:
    every time next_key is called we fetch the next in overlay and the next in backend. But the backend doesn't implement any cache for key iteration. a quick cache could be implemented in Ext implementation of Externalities, but then if a user iterate on two structure at the same time this cache is useless. A proper cache should be implemented in a lower layer.

  • the sr-io API could include the suffix like next_key_with_suffix(key, suffix) -> Option<key> so that if we have to implement some cache we could limit the number of key in advance we cache.

    note: in the current architecture with BTreeMap for changeset this is not needed
    note: if in the future we have to change this overlay we can still also provide a new call with this suffix for an optimized call.

  • benchmark to see if BTreeMap is ok, otherswise we can try other solution with some cache on the hashmap. Some benchmark between BTreeMap and Hashmap are there #4185 (comment)

Fix #3883

@thiolliere

This comment has been minimized.

Copy link
Contributor Author

thiolliere commented Nov 22, 2019

Iteration on overlay by changing Hashmap to BTreeMap:

Early benchmark between BTreeMap and HashMap:
each map are filled with random key of size 32bytes, we remove 100 existing keys or insert 100 random keys or get 100 existing keys. The result shows:

if maps contain less than 10_000 key/values then performance are quite same.
if maps contain 100_000 key/values then performance of BTreeMap is almost 2 times slower than HashMap.

benchmark.txt
result.txt

Iteration on overlay with cache.

If ext_call specifies the range on which it will iterate

for example get_in_range(from_key, until_key, number_of_key_to_return) -> Vec<Key>
then we can cache all the keys from the last returned (not included) to until_key. then if the cache (like a key is inserted or removed) we can reduce the cache to the cleand part (but then this cache reducing operation happens always). we remove the cache entirely when it is touched.

If ext_call doesn't specifies the range on which it will iterate

then the only way to make a cache that is useful is to reduce the cache to the not dirty part when the storage gets dirty.

@thiolliere

This comment has been minimized.

Copy link
Contributor Author

thiolliere commented Nov 26, 2019

implementation is almost complete, still todo:

  • optimisation of next_key in overlay.
  • storage cache in client
  • implement next_key for child storage (easy)
@xlc

This comment has been minimized.

Copy link
Contributor

xlc commented Nov 27, 2019

How likely is this going to be merged with in next two weeks? Asking because I am writing materials for my Substrate course and if this is going to land before the course start, I can drop the linked_map part and teach prefixed map instead.

@thiolliere thiolliere force-pushed the gui-prefixed-map-2 branch from b1934c6 to 5c8d71a Nov 27, 2019
@thiolliere thiolliere changed the title Introduce prefixed storage without enumeration Introduce prefixed storage with enumeration Nov 27, 2019
@thiolliere thiolliere marked this pull request as ready for review Nov 29, 2019
@thiolliere thiolliere requested review from kianenigma and pepyakin as code owners Nov 29, 2019
@thiolliere

This comment has been minimized.

Copy link
Contributor Author

thiolliere commented Nov 29, 2019

How likely is this going to be merged with in next two weeks? Asking because I am writing materials for my Substrate course and if this is going to land before the course start, I can drop the linked_map part and teach prefixed map instead.

The PRs waiting for this are no longer waiting, but it seems a very useful feature. About the deadline, it is mostly about if performance are OK by switching HashMap to BTreeMap, or if we need better and different cache and still use HashMap

@gavofyork

This comment has been minimized.

Copy link
Member

gavofyork commented Dec 1, 2019

it's acceptable, though we might want to revert to hashmap and add a cache at a later date if real world usage dictates. as long as the API is fixed and we can change as desired later, then we're good.

@thiolliere

This comment has been minimized.

Copy link
Contributor Author

thiolliere commented Dec 3, 2019

yes understood,

hmm then maybe it worth making the sr-io API next_key_with_prefix(next_key: &[u8], prefix: &[u8]) so that if we have to implement a cache then the runtime can provide the prefix he wants to iterate on, allowing not to create a big cache when not needed.

EDIT: top description updated including this thought

@gavofyork gavofyork added A6-seemsok and removed A0-pleasereview labels Dec 5, 2019
@gavofyork

This comment has been minimized.

Copy link
Member

gavofyork commented Dec 5, 2019

Shortly after this goes in we will want to consider adding to storage maps the possibility to prefix the unhashed key to the value, thereby allowing the possibility of enumeration over all keys from both the runtime and UI. Right now to do that we only really have the option of using twox(key)++key hasher which isn't especially secure against unbalanced trie griefing.

@gavofyork

This comment has been minimized.

Copy link
Member

gavofyork commented Dec 6, 2019

anyone else want to sign this off before merge?

@bkchr

This comment has been minimized.

Copy link
Contributor

bkchr commented Dec 6, 2019

I will review it now.

@bkchr
bkchr approved these changes Dec 6, 2019
// The key just after the one given in input, basically `key++0`.
// Note: We are sure this is the next key if:
// * size of key has no limit (i.e. we can always add 0 to the path),
// * and no keys can be inserted between `key` and `key++0` (this is ensured by sr-io).

This comment has been minimized.

Copy link
@bkchr

bkchr Dec 6, 2019

Contributor

What does this mean and how is that ensured?

This comment has been minimized.

Copy link
@thiolliere

thiolliere Dec 9, 2019

Author Contributor

we doesn't want to iterate from the key included but from the key excluded and trie crate only provides seek function.

So we compute the next potential key, by doing next_potential_key = key++0.

But this is only true if:

  • we can indeed add 0 to the key
  • and if there is no key in the trie between key and key++0

First option requires size of key has no limit or that the runtime is not suppose to write anything near this limit, and the second option is ensured because sr-io doesn't provide any way to write between key and key++0, even if it is a 16-trie.

Maybe @cheme you can confirm this ?

But maybe it is better to actually don't make those assumption and seek from the key, get the next, if the next is the current one then get the second next. or implement some new features in trie.

This comment has been minimized.

Copy link
@thiolliere

thiolliere Dec 9, 2019

Author Contributor

that would result in something like this:

diff --git a/primitives/state-machine/src/trie_backend_essence.rs b/primitives/state-machine/src/trie_backend_essence.rs
index aea0193dd..1c8bafb09 100644
--- a/primitives/state-machine/src/trie_backend_essence.rs
+++ b/primitives/state-machine/src/trie_backend_essence.rs
@@ -110,23 +110,29 @@ impl<S: TrieBackendStorage<H>, H: Hasher> TrieBackendEssence<S, H> where H::Out:
                let mut iter = trie.iter()
                        .map_err(|e| format!("TrieDB iteration error: {}", e))?;
 
-               // The key just after the one given in input, basically `key++0`.
-               // Note: We are sure this is the next key if:
-               // * size of key has no limit (i.e. we can always add 0 to the path),
-               // * and no keys can be inserted between `key` and `key++0` (this is ensured by sr-io).
-               let mut potential_next_key = Vec::with_capacity(key.len() + 1);
-               potential_next_key.extend_from_slice(key);
-               potential_next_key.push(0);
-
-               iter.seek(&potential_next_key)
+               iter.seek(&key)
                        .map_err(|e| format!("TrieDB iterator seek error: {}", e))?;
 
                let next_element = iter.next();
 
                let next_key = if let Some(next_element) = next_element {
-                       let (next_key, _) = next_element
-                               .map_err(|e| format!("TrieDB iterator next error: {}", e))?;
-                       Some(next_key)
+                       let next_key = next_element
+                               .map_err(|e| format!("TrieDB iterator next error: {}", e))?.0;
+
+                       if next_key != key {
+                               Some(next_key)
+                       // If next key is same as key iter to second next element.
+                       } else {
+                               let next_element = iter.next();
+                               if let Some(next_element) = next_element {
+                                       let next_key = next_element
+                                               .map_err(|e| format!("TrieDB iterator next error: {}", e))?.0;
+
+                                       Some(next_key)
+                               } else {
+                                       None
+                               }
+                       }
                } else {
                        None
                };

This comment has been minimized.

Copy link
@cheme

cheme Dec 9, 2019

Contributor

Yes, the fact that api only allow to write key value at byte address is important here (otherwhise we would need to work with nibbles (half byte for 16-trie) and add 0 nibble.
I am not sure if skipping the first iteration is very important here (we need to query the node anyway during seek call).
The relevant perf gain would be from using the same iterator between calls (to skip the 'seek' call on every 'next), but it is not a easy todo.

This comment has been minimized.

Copy link
@thiolliere

thiolliere Dec 9, 2019

Author Contributor

Should we not rely on "the fact that api only allow to write key value at byte address", this property could be leverage in the future.

@shawntabrizi

This comment has been minimized.

Copy link
Member

shawntabrizi commented Dec 9, 2019

Is it correct that after this PR is merged, linked_map could be entirely replaced with map?

@thiolliere

This comment has been minimized.

Copy link
Contributor Author

thiolliere commented Dec 9, 2019

Is it correct that after this PR is merged, linked_map is obsolete?

To make it "really" obsolete we need to provide an easy to make maps storing their key alongside the value as Gav says here: #4185 (comment)

@bkchr

This comment has been minimized.

Copy link
Contributor

bkchr commented Dec 9, 2019

Is it correct that after this PR is merged, linked_map could be entirely replaced with map?

Maps are also not sorted, like linked maps.

primitives/sr-io/src/lib.rs Outdated Show resolved Hide resolved
@bkchr bkchr merged commit 6b70f12 into master Dec 9, 2019
13 of 14 checks passed
13 of 14 checks passed
continuous-integration/gitlab-check_polkadot Build stage: build; status: failed
Details
continuous-integration/gitlab-cargo-check-benches Build stage: test; status: success
Details
continuous-integration/gitlab-cargo-check-subkey Build stage: test; status: success
Details
continuous-integration/gitlab-check-line-width Build stage: test; status: success
Details
continuous-integration/gitlab-check-runtime Build stage: test; status: success
Details
continuous-integration/gitlab-check-web-wasm Build stage: test; status: success
Details
continuous-integration/gitlab-check_warnings Build stage: build; status: success
Details
continuous-integration/gitlab-node-exits Build stage: test; status: success
Details
continuous-integration/gitlab-test-dependency-rules Build stage: test; status: success
Details
continuous-integration/gitlab-test-frame-staking Build stage: test; status: success
Details
continuous-integration/gitlab-test-full-crypto-feature Build stage: test; status: success
Details
continuous-integration/gitlab-test-linux-stable Build stage: test; status: success
Details
continuous-integration/gitlab-test-linux-stable-int Build stage: test; status: success
Details
continuous-integration/gitlab-test-wasmtime Build stage: test; status: success
Details
@bkchr bkchr deleted the gui-prefixed-map-2 branch Dec 9, 2019
@thiolliere thiolliere mentioned this pull request Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.