feat(store): UnorderedMap v2 implementation #584

austinabell · 2021-09-23T15:01:57Z

Closes #580

Also removes some unnecessary bounds in LookupMap and replaces the unreachable!() code with aborts

…stin/store/imap

…austin/store/bucket

near-sdk/src/store/unordered_map/mod.rs

austinabell · 2021-09-23T20:53:19Z

Note to self, there is a TODO added with the ability to make clearing more efficient. Going to do this in parallel (just optimizes away a clone and adds drain iterator for Vector so it doesn't block this, but keep in mind this will change

edit: change started here #592

ChaoticTempest

LGTM

…e/unordered_map

austinabell · 2021-09-28T19:32:00Z

This is a pretty crucial data structure, and changes are pretty involved, so I'm going to wait for at least one more review on this

matklad · 2021-09-30T13:38:14Z

near-sdk/src/store/lookup_map/impls.rs

+    /// # Panics
+    ///
+    /// Panics if the key does not exist in the map
+    fn index_mut(&mut self, index: &Q) -> &mut Self::Output {


Note that std's maps don't implement index_mut, so, if we are shooting for consistency, we might want to avoid it as well.

Tough, I think the reason for why there are no index-muts is just forward compatibility (basically, folks want that hm[new_key] = new_value inserted new value, instead of panicking), so there's a reasoning that, as we, unlike std, can in theory release new major version, we might add IndexMut.

Not sure, though I'd go for conservative and "obvious" solution of just following std here.

Ah, that's a good point. I'll leave them out for now, we can decide if we want it to add an entry or panic on not exist later

matklad · 2021-09-30T13:38:52Z

near-sdk/src/store/mod.rs

@@ -10,11 +10,13 @@ pub use vec::Vector;
 pub mod lookup_map;
 pub use self::lookup_map::LookupMap;

+pub mod unordered_map;
+pub use self::unordered_map::UnorderedMap;


Heh, still reeealy don't like C++ish unordered_map naming :)

yeah, I'm not a huge fan either but decided to keep existing naming to have an easier transition from the existing type. Are you suggesting having it be named HashMap, StorageHashMap or something like this? I don't have an opinion on naming

cc: @ChaoticTempest @mikedotexe

:) I don't have a strong opinion on this, but we do reference UnorderedMap in a lot of places and the dev community might be used to it

matklad · 2021-09-30T13:41:34Z

near-sdk/src/store/unordered_map/iter.rs

+{
+    /// Values iterator which contains empty and filled cells.
+    keys: bucket::Iter<'a, K>,
+    /// Amount of valid elements left to iterate.


Doc comment feels like it is wrong now? Can't connect Amount with this being a LookupMap.

ah, yes, just forgot to come back to fix this

matklad · 2021-09-30T13:49:54Z

near-sdk/src/store/bucket/mod.rs

+                        Op::Clear => {
+                            sv.clear();
+                            hm.clear();
+                        }
                    }
                }
            }


Unrelated to the PR, but I'd use first_free/ next_free in Bucket, rather than next_vacant / next_index. Also like Slot terminology for container, as in

https://github.com/DataDog/glommio/blob/master/glommio/src/free_list.rs#L40-L44

That's fine, I'll change now. All private APIs and that change has already come in. Technically it isn't first_free as this points to the last index removed so I will adjust naming from your suggestion slightly. Do you think the Bucket type should be changed to something resembling FreeList? It's private and hasn't been released yet so can easily change

as this points to the last index removed

Last index removed is the first free index (in a free list) :) As for namling, I personally have a very strong association that a vec of slots with index-based list of free slots is a free list, but this might be idiosincratic usage. Doesn't really matter in the grand scheme of things, just wanted to share some conventions I developed around this data structure (as I think I've had to code a Rust free list a dozen of times, its a farily frequent thing).

matklad · 2021-09-30T13:51:05Z

near-sdk/src/store/lookup_map/impls.rs

+    K: BorshSerialize + Ord + Clone + Borrow<Q>,
+    V: BorshSerialize + BorshDeserialize,
+    H: CryptoHasher<Digest = [u8; 32]>,
+    Q: BorshSerialize + ToOwned<Owned = K>,


Kindof not a huge fan of the amount of bounds here -- would look bad as a public API in rustdoc. Otoh, not that I can suggest a better alternative :)

matklad · 2021-09-30T13:52:34Z

near-sdk/src/store/unordered_map/iter.rs

+use super::{CryptoHasher, LookupMap, UnorderedMap, ValueAndIndex, ERR_INCONSISTENT_STATE};
+use crate::{env, store::bucket};
+
+impl<'a, K, V, H> IntoIterator for &'a UnorderedMap<K, V, H>


Doc for UnorderedMap says its not itrable (docs are wrogn)

I see that in the docs, unless you are looking for something that I'm not?

Wait, we have two UnorderedMaps as of 704cb26? The one in store/unordered_map claims that it is not iterable. But here we implement the iter for that map.

The one in collections/unordered_map does say that it is iterable.

https://github.com/matklad/near-sdk-rs/blob/704cb26de463bc9756d38f4d3694bc0d288d2933/near-sdk/src/store/unordered_map/iter.rs#L8

https://github.com/matklad/near-sdk-rs/blob/704cb26de463bc9756d38f4d3694bc0d288d2933/near-sdk/src/store/unordered_map/mod.rs#L21

the two links I am looking at

Ah, yeah that was just a detail missed when copying over the API that I hadn't updated. It's fixed now

matklad · 2021-09-30T13:55:19Z

CHANGELOG.md

  - When mixing using `sys` and `env`, reduces chance of collision for using `0`
+- store: Implement caching `LookupMap` type. This is the new iteration of the previous version of `near_sdk::collections::LookupMap` that has an updated API, and is located at `near_sdk::store::LookupMap`. [PR 487](https://github.com/near/near-sdk-rs/pull/487).


It took me a second te realize what's the difference between lookup map and unordered map (that is, that um hashes the keys to get the trie key). We might explain this better in the docs.

well both do, the only difference is that unordered map keeps track of keys in this vector like structure so that it can be iterated (LookupMap isn't aware of what elements exist in it). I'll try to make docs more clear

Reading the docs, I was lazy with re-reading this. My fault. Used LookupMap as a framework since this is a superset of the functionality, but didn't make a pass through the docs.

matklad · 2021-09-30T14:05:50Z

near-sdk/src/store/unordered_map/mod.rs

+    H: CryptoHasher<Digest = [u8; 32]>,
+{
+    keys: Bucket<K>,
+    values: LookupMap<K, ValueAndIndex<V>, H>,


🤔 hm, I'd naively expect that we us the following repr here:

struct UnordereMap { lm: LookupMap<CryptoHash, (K, V)> }

ie, that we just use hashes as keys and key-value pairs as values.

Which also makes me think, that maybe lookup map is the wrong abstaction? Ie, the difference between lookup map and unordered map is that the former uses borsh serialize as the trie key, while the latter uses sha hash as the key.

So perhaps we need to introduce some kind of RawSet internal abstraction that just stores byte keys and values and parametrized by how we get thouse bytes out of Rust types.

Than lookup map would be one specialization that uses Borsh for both, and UnorderedMap would use Borsh + CryptoHasher.

Nah, they both use the hash as the key for lookup. This does make me think I should refactor the LookupMap to remove keeping the owned values of the types at all. Probably would be lighter and better to build off of.

I can't remember as I'm responding to this what the specific reason was for this in the first place.

Edit: The reason was that you could avoid hashing a key altogether if something was added then deleted. Also, for looking up values you only need to hash when the element is not in cache. This should probably be benchmarked with actual tracking of gas usage to see which would be better.

Yeah, I guess I should make a more careful pass over it, as it seems I got myself confused about what this is doing at all :)

Opened #596 which might make things more clear and be a higher leverage piece of code to weigh in on

…e/unordered_map

austinabell added 25 commits September 20, 2021 17:17

test(vector): Add fuzz test for Vector

2f069e9

refactor(store): Move internal vec functionality into index map

17406c3

fmt and lint

015834f

remove empty test

26f5516

Update docs

bca606d

optimize lookup key

023d34d

Merge branch 'master' into austin/vec/fuzz

fe8eddd

Merge branch 'austin/vec/fuzz' of github.com:near/near-sdk-rs into au…

2ade7a8

…stin/store/imap

fmt

e483edb

feat: Implement basic bucket type

c551718

impl iterators

5151cd0

testing, docs, fixes

0f38b1a

add reset option for test, to simulate serializing and deserializing

f0eba3f

Merge branch 'austin/vec/fuzz' of github.com:near/near-sdk-rs into au…

353cf73

…stin/store/imap

Merge branch 'austin/store/imap' of github.com:near/near-sdk-rs into …

681c9be

…austin/store/bucket

feat(store): UnorderedMap implementation

ca3a694

immutable iterator

2e50c1b

iter mut impl

4c97c7a

optimize iterator loading

031f206

add remove_entry function (matching hashmap)

e62182f

add keys iterator

1c43ad7

values iterator

831da81

values mut iterator

b39f73c

update iterator test to include new iterator functions

e7eeb30

remove unnecessary bounds

d7ccda9

austinabell requested review from ChaoticTempest, evgenykuzyakov and mikedotexe as code owners September 23, 2021 15:01

ocd

1f902b9

austinabell commented Sep 23, 2021

View reviewed changes

near-sdk/src/store/unordered_map/mod.rs Outdated Show resolved Hide resolved

austinabell added 2 commits September 23, 2021 13:45

Update near-sdk/src/store/unordered_map/mod.rs

dbe3ef9

add clear method, fix bucket bug, add tests

4acebc7

ChaoticTempest approved these changes Sep 23, 2021

View reviewed changes

austinabell mentioned this pull request Sep 24, 2021

feat(vector): Implement drain iterator for Vector #592

Merged

Update changelog

a600cf5

Base automatically changed from austin/store/bucket to master September 28, 2021 19:17

austinabell added 2 commits September 28, 2021 15:20

Merge branch 'master' of github.com:near/near-sdk-rs into austin/stor…

65b1528

…e/unordered_map

delete re-added file

704cb26

matklad reviewed Sep 30, 2021

View reviewed changes

austinabell added 4 commits September 30, 2021 10:39

update some docs

39bb129

remove indexmut impls

35c7d40

remove index mut use in tests

58afc73

Rename Bucket to FreeList

93afb1d

mikedotexe approved these changes Oct 1, 2021

View reviewed changes

austinabell mentioned this pull request Oct 1, 2021

v2 TreeMap #588

Closed

austinabell added 3 commits October 5, 2021 11:44

rename field to first_free

5127798

Merge branch 'master' of github.com:near/near-sdk-rs into austin/stor…

2bc35b7

…e/unordered_map

Merge branch 'master' into austin/store/unordered_map

69dbe93

austinabell merged commit 610cdf7 into master Oct 7, 2021

austinabell deleted the austin/store/unordered_map branch October 7, 2021 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(store): UnorderedMap v2 implementation #584

feat(store): UnorderedMap v2 implementation #584

austinabell commented Sep 23, 2021

austinabell commented Sep 23, 2021 •

edited

Loading

ChaoticTempest left a comment

austinabell commented Sep 28, 2021

matklad Sep 30, 2021

austinabell Sep 30, 2021

matklad Sep 30, 2021

austinabell Sep 30, 2021

mikedotexe Oct 1, 2021

matklad Sep 30, 2021

austinabell Sep 30, 2021

matklad Sep 30, 2021

austinabell Sep 30, 2021 •

edited

Loading

matklad Sep 30, 2021

matklad Sep 30, 2021

matklad Sep 30, 2021

austinabell Sep 30, 2021

matklad Sep 30, 2021

austinabell Sep 30, 2021

matklad Sep 30, 2021

austinabell Sep 30, 2021

austinabell Sep 30, 2021

matklad Sep 30, 2021

austinabell Sep 30, 2021 •

edited

Loading

matklad Sep 30, 2021

austinabell Sep 30, 2021

		- When mixing using `sys` and `env`, reduces chance of collision for using `0`
		- store: Implement caching `LookupMap` type. This is the new iteration of the previous version of `near_sdk::collections::LookupMap` that has an updated API, and is located at `near_sdk::store::LookupMap`. [PR 487](https://github.com/near/near-sdk-rs/pull/487).

feat(store): UnorderedMap v2 implementation #584

feat(store): UnorderedMap v2 implementation #584

Conversation

austinabell commented Sep 23, 2021

austinabell commented Sep 23, 2021 • edited Loading

ChaoticTempest left a comment

Choose a reason for hiding this comment

austinabell commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell commented Sep 23, 2021 •

edited

Loading

austinabell Sep 30, 2021 •

edited

Loading

austinabell Sep 30, 2021 •

edited

Loading