feat: caching LookupMap implementation #487

austinabell · 2021-07-21T01:14:09Z

The interface closely matches a std::collections::HashMap without the functions that require being aware of all keys (as this does not store keys, another data structure will be built on this for that) and adding on functions that are useful in this context such as:

set: update value without loading the existing value from state and returning replaced
flush: To write cached modifications to storage manually (done automatically on Drop, but this could be useful for if measuring storage usage before and after and not requiring drop)

ChaoticTempest

just some comments about naming, but overall looking good

ChaoticTempest · 2021-09-09T01:11:32Z

near-sdk/src/utils/stable_map.rs

@@ -40,4 +41,12 @@ impl<K, V> StableMap<K, V> {
    pub(crate) fn inner(&mut self) -> &mut BTreeMap<K, Box<V>> {
        self.map.get_mut()
    }
+    pub(crate) fn with_value<Q: ?Sized, F, T>(&self, k: &Q, f: F) -> Option<T>


I don't think we should prefix this function with with_ since those have been more primarily associated with the creation of the struct itself. Maybe name it something like map_value?

My thinking was that this is not actually mapping the value since it's just calling a function on the reference to the value, not moving the owned value to be mapped. This might just be semantics, but map would be FnOnce(V) -> T when this is actually FnOnce(&V) -> T

If you feel strongly about this I'm happy to change since I don't really have an opinion here and it's a private API

ahh I see. I don't have too strong of an opinion what this should be either. And huh, VaListImpl::with_copy has a similar signature, so it's not too bad for it to have a prefix of with_ but certainly very uncommon

renamed to map_value_ref. I don't really think there is a great name, but it's internal and this might be the clearest. Sound good to you?

Yup that sounds good to me. Thanks

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

matklad

Did a very quick look, will continue tomorrow!

matklad · 2021-09-14T16:45:28Z

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

+    /// assert_eq!(map["poneyland"], "hoho".to_string());
+    /// ```
+    pub fn or_insert_with<F: FnOnce() -> V>(self, default: F) -> &'a mut V {
+        match self {


this could delegate to or_insert_with_key

Does this get compiled away? I just noticed that std::collections::HashMap does not do this and decided to follow their pattern.

Yes, according to my mental model, the compiler should trivially compile that away. This pattern is used by Option and once cell: https://github.com/rust-lang/rust/blob/cdeba02ff71416e014f7130f75166890688be986/library/core/src/option.rs#L1228

It's interesting that maps don't do that. I suppose that's historical -- this is the form the API was added before 1.0, and the form it was copy-pasted into hash brown: rust-lang/rust#22930.

No strong opinion here though -- if you'll feel more confident with the current impl, that's totally fine!

No, that's fine, I'll switch. I'm just noting the reason I had was that I assumed because it was the pattern in std::.::HashMap that there was a benefit

matklad · 2021-09-14T16:46:31Z

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

+        match self {
+            Self::Occupied(entry) => entry.into_mut(),
+            Self::Vacant(entry) => entry.insert(default),
+        }


This can delegate to or_insert_with

Suggested change

match self {

Self::Occupied(entry) => entry.into_mut(),

Self::Vacant(entry) => entry.insert(default),

}

self.or_insert_with(|| default)

That way, there's a single place which contains the match.

same here as above, I'm following std::HashMap pattern. I'll try to dig into comparing what the two compile to exactly to make sure there is no difference

matklad · 2021-09-14T16:55:22Z

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

+        K: Borrow<Q>,
+        Q: BorshSerialize + ToOwned<Owned = K> + Ord,


Huh, that's subtle!

I think here we have an extra requrenment that Q: BorshSerialize and K: BorshSerialize use the same serialization format. IE, this extends the usual Borrow contract to cover serialization format as well.

I don't see any immediate problems here.

yeah, it's not ideal. I couldn't think of a cleaner way to handle this, though. Maybe we can document just to make sure this assumption is clear. Assuming K and Borrow<K> have the same serialization seems in line with how Borrow is defined in some places, but others indicate that it only covers Hash, Eq, Ord

near-sdk/src/environment/hash.rs

near-sdk/src/lib.rs

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

matklad · 2021-09-15T11:07:55Z

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

+    where
+        K: Clone,


suggestion(blocking for stabilization): the K: Clone bound doesn't make sense -- we already have an owned K here. Can we write

impl StableMap { fn get_key_value_mut(&mut self, k: K) -> (&mut K, &mut V) { ... } }

sounds doable in theory?

Intuitively, it makes sense, but I don't know how you're getting the &mut K, let alone a &K through the entry API like this.

I'll come back to this to see if there is anything that can be done, but at best with some code duplication all I can come up with is removing one clone, which I'm not certain is very helpful

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

matklad

Didn't do a second pass through, but this certainly feel ready as an unstable feature.

* master: feat: caching LookupMap implementation (#487)

austinabell and others added 30 commits May 4, 2021 09:35

add unstable feature

1e90139

feat: setup unstable vec collection and basic implementations

1446ad0

setup framework for caching values (wip and unsafe)

babd92e

fmt and update pop impl

c2e39b8

Merge branch 'master' into unstable_vec

6efbc64

implement load methods

13e0662

implement swap remove

63de2af

finish base implementation

b884f32

implement iterator functionality

e5e1ca8

replace casting as potential panic

b35a49b

implement index trait impls

4f360b8

port over existing tests

38cc873

swap some tests to use new APIs to test

486cd46

update debug

5aed715

remove duplicate extend and fix modified match (oops)

c0d088d

this is ugly but seems to solve unsoundness issue

8fb7324

Merge branch 'master' of github.com:near/near-sdk-rs into unstable_vec

e56b207

migrate vec to store module

a6ba75b

Merge branch 'master' of github.com:near/near-sdk-rs into unstable_vec

75ffd20

refactor storage load deserialization to fix bug and cleanup

1335407

add safety doc on load

b378bf4

refactor: update cache interface to avoid unsafe usages

7a7de54

update docs

e67dc3f

move stable map to utils

c51b1ce

fmt

fffc91f

Merge branch 'master' into unstable_vec

39648fa

test and fix iterator logic and panic message

82eb8fa

switch iterator range to use ops::Range

2f602b9

update lifetime cast on iter mut function

7dc6049

reuse buffer for serialization on flush (thanks matklad)

9e54bb6

austinabell requested a review from mikedotexe as a code owner September 8, 2021 22:57

basic debug impl

9069cf1

ChaoticTempest requested changes Sep 13, 2021

View reviewed changes

update with_hasher name and docs spacing

0d73425

matklad reviewed Sep 14, 2021

View reviewed changes

matklad reviewed Sep 15, 2021

View reviewed changes

austinabell added 7 commits September 16, 2021 17:36

forward entry insert methods to eachother

b42e2ae

switch fixed register loading to use maybeuninit

f281ef1

cleanup from review

5d3db04

seal cryptohash trait

34b75f5

add fuzz test

be564e8

fix docs for rename

bbef818

Merge branch 'master' into austin/hashmap

699b16a

austinabell requested a review from ChaoticTempest September 17, 2021 00:28

austinabell added 7 commits September 16, 2021 20:29

modify insert test to be less bad

e0af103

cache calculated prefix hash

1f525c8

remove unnecessary references

d602834

fix clippy lints and add unstable to CI

79d2ab7

fmt

7b54247

reuse buffer for lookup key

62ca86a

update fuzz checks to include set calls

fff8727

austinabell requested a review from matklad September 20, 2021 16:53

rename callback function on stable map

a3e4383

matklad approved these changes Sep 21, 2021

View reviewed changes

ChaoticTempest approved these changes Sep 21, 2021

View reviewed changes

austinabell merged commit e888599 into master Sep 21, 2021

austinabell deleted the austin/hashmap branch September 21, 2021 19:46

This was referenced Sep 22, 2021

v2 store UnorderedMap implementation #580

Closed

v2 LookupSet #586

Closed

chadoh added a commit that referenced this pull request Sep 23, 2021

Merge branch 'master' into feat/near-runner-jest

b2c8e24

* master: feat: caching LookupMap implementation (#487)

feat: caching LookupMap implementation #487

feat: caching LookupMap implementation #487

Uh oh!

Conversation

austinabell commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChaoticTempest left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matklad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matklad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

austinabell commented Jul 21, 2021 •

edited

Loading