refactor: move nodes counter to the caching storage #6315

Longarithm · 2022-02-17T16:15:48Z

Work on #6241 showed the following issues with trie counter:

it's implemented as atomic, but using it in different threads doesn't make sense;
node accounting logic makes more sense nearly to caching, because we use it only on TrieCachingStorage and the chunk nodes cache feature will impact it as well.

So here we move counter to the TrieCachingStorage and remove some unnecessary counter usages.

Testing

Add currently missing tests:

for counting touched nodes and trie cache size;
same but for repeated values.

matklad · 2022-02-17T16:19:19Z

core/store/src/trie/mod.rs

@@ -577,7 +573,6 @@ impl Trie {
        if *hash == Trie::empty_root() {
            Ok(memory.store(TrieNodeWithSize::empty()))
        } else {
-            self.counter.increment();


Do we have any protocol changes here? In particular, I see there's self.storage.retrieve_raw_bytes(hash)?; in delete_value, and that now would increment the counter.

Great question!

From what I see, all logic leading to incrementing counter here happens in Trie::update which itself is called after all tx/receipt execution. I feel that some counter increments makes us think that we charge for them somewhere, although we don't do it in reality. So it's better to remove them. But I also will be happy if someone double-check this.

Is there a way to test this?

I think that what exactly we need to test is that gas costs were not affected by this change - because they are part of protocol.

The similar purpose is served by the test group next to Runtime implementation. In particular, they check gas costs after Runtime::apply() call, which include Trie::update call.

However, we currently have only one test applying funcall txs - test_apply_deficit_gas_for_function_call_partial. So I will add a new test there which will test a funcall which touches contract storage. This should help in the future - if we accidentally change counter, the test must fail.

UPD: added a test test_contract_read_write_cost

@Longarithm I still don't understand why the change in delete_value won't cause any change in gas counting. Could you elaborate more on that?

One way to test this manually, is maybe run state viewer apply chain on some mainnet blocks to verify that gas counting doesn't change.

@mzhangmzz delete_value remains unchanged here, it is a change in move_node_to_mutable.

Anyway it won't cause changes in gas counting, because these methods are called only on Trie::update, which happens after tx/receipt processing as I said in the second comment. Having the increment I am removing actually didn't make much sense.

I think delete_value will change here, because previously it only calls self.storage.retrieve_raw_bytes(hash)? without incrementing the counter. After your change, since you moved the counter incrementing logic inside storage.retrieve_raw_bytes, delete_value will increase the counter.

I'm not super familiar with the gas counting logic, so I'm not sure why it is called in Trie::update won't affect gas counting.

Yeah, you are right.

I'm not sure why it is called in Trie::update won't affect gas counting.

I've looked over all Trie::update calls. It is called for four purposes:

tests

splitting states (add_values_to_split_states_impl)

applying genesis (finalize_genesis)

in TrieUpdate::finalize

The latter function is called for similar purposes - I think it doesn't make much sense to list all them. Also it is called in the end of Runtime::apply to apply changes to trie, when all tx/receipts are already executed and burnt/used gas is computed.

Moreover, trie counter is used for gas counting only during function call execution, which is triggered only by process_receipt, this is another reason why Trie::update change doesn't affect it.

Sorry I didn't explain it before properly. On my side, it was a fast look over CLion usages, so I thought it didn't worth attention.

Also, I could leave the incrementation logic as is. But it would make the implementation more ugly, because by default TrieStorage shouldn't know about counter.

core/store/src/trie/mod.rs

core/store/src/trie/trie_storage.rs

core/store/src/trie/trie_tests.rs

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

bowenwang1996

@EgorKulikov @mzhangmzz please review the PR

bowenwang1996 · 2022-02-21T23:49:08Z

core/store/src/trie/mod.rs

@@ -577,7 +573,6 @@ impl Trie {
        if *hash == Trie::empty_root() {
            Ok(memory.store(TrieNodeWithSize::empty()))
        } else {
-            self.counter.increment();


Is there a way to test this?

mzhangmzz

Will approve to unblock. I suggest that as a final test, run state-viewer apply-chain on a mainnet node to make sure the gas counting logic doesn't change.

(I'm convinced that it didn't. But this part of logic is quite complicated and I just want to be sure)

core/store/src/trie/trie_storage.rs

mm-near · 2022-03-07T12:34:30Z

core/store/src/trie/mod.rs

@@ -612,16 +606,11 @@ impl Trie {
        }
    }

-    pub(crate) fn retrieve_raw_bytes(&self, hash: &CryptoHash) -> Result<Arc<[u8]>, StorageError> {


@matklad - more Rust question:

in such case - should we 'keep' this method (and have it simply call self.storage.retrieve_raw_bytes) or should we allow other classes to call XX.storage.retrieve_raw_bytes (as Alex did in this PR)?

For me - one advantage of keeping this method - is that we keep better 'layers' - and avoid having other classes 'peek' into Trie) - but I wonder if it has some performance implications

@mm-near there won't be performance implications here. In general, it's a pretty safe assumption in Rust that linguistic abstractions don't introduce overhead -- static nature of the language and unboxed memory layout guarantee that compiler "sees through" any abstractions.

Regarding layering, my general observation is that layering is good only when it is enforced: it's better to focus on specific places where layering is important, rather than to try to enforce it recursively throughout the stack. There should be places where there's explicitly no layers.

For Rust, the best enforcement is aligning layers with crate boundaries: that's why I am relatively picky about pub vs pub(crate), but relatively indifferent between pub(crate) and /*private*/.

As an aside, I think we can make storage private rather pub(crate) if we put trie tests into submodule of the trie module. We only need pub(crate) for tests, but "pub(crate) for tests" is a bit of oxymoron -- pub(crate) guarantees that this is for unit tests, and unit tests can always be made a submodule (in Rust, submodule see the full contents of partent modules)

Longarithm · 2022-03-09T21:16:11Z

@mzhangmzz I run state-viewer on blocks [55290778, 55290878) on Aurora shard - sequence from the end of my DB snapshot - showed full equality of both burnt_gas and touching_trie_node_cost for all receipts. Same for shard 0.

I attach the logs for Aurora shard:
logs.zip

Longarithm added 6 commits February 17, 2022 18:41

move counter and implement test

a71e23b

compilation fix

3b86172

compilation fix

52d5c59

compilation fix

7c587df

compilation fix

df07bb9

compilation fix

c9db0d1

Longarithm requested review from mzhangmzz, bowenwang1996, matklad, mm-near and EgorKulikov as code owners February 17, 2022 16:15

matklad reviewed Feb 17, 2022

View reviewed changes

core/store/src/trie/mod.rs Outdated Show resolved Hide resolved

matklad reviewed Feb 17, 2022

View reviewed changes

core/store/src/trie/trie_storage.rs Outdated Show resolved Hide resolved

matklad reviewed Feb 17, 2022

View reviewed changes

core/store/src/trie/trie_tests.rs Outdated Show resolved Hide resolved

core/store/src/trie/trie_tests.rs Outdated Show resolved Hide resolved

Longarithm and others added 3 commits February 17, 2022 19:44

Update core/store/src/trie/trie_storage.rs

ffab524

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

apply suggestions

6b5327d

Merge branch 'master' into move-counter

c06f983

Longarithm requested a review from matklad February 17, 2022 17:19

add test with repeated values

72c0be1

Longarithm force-pushed the move-counter branch from a63063d to 72c0be1 Compare February 17, 2022 22:17

Longarithm added 2 commits February 18, 2022 16:03

merge with master

ad2fc60

cargo fmt fix

0517369

bowenwang1996 reviewed Feb 21, 2022

View reviewed changes

Longarithm added 6 commits February 23, 2022 01:51

add test contract rw cost test

5777028

compilation fix

d168d01

compilation fix

6f3c509

compilation fix

31e0471

compilation fix

3eabc3b

compilation fix

52d7bda

Longarithm and others added 9 commits February 23, 2022 02:24

compilation fix

6f4c37d

cost name fix

2c1ae5b

set value for test

b60fcf0

Merge branch 'master' into move-counter

a66b203

debug print

63a700b

debug

0336b32

debug

314c3f3

debug

7aaf621

debug

9017bd4

Longarithm requested review from olonho and nagisa as code owners February 23, 2022 21:30

Longarithm added 3 commits February 24, 2022 00:34

debug

de97807

debug

118835d

remove debug

c4744a7

Longarithm requested review from bowenwang1996 and removed request for nagisa and olonho February 23, 2022 22:10

Longarithm added 2 commits February 24, 2022 01:13

remove debug

df58c0f

remove debug

bb6ecba

mzhangmzz approved these changes Feb 25, 2022

View reviewed changes

matklad approved these changes Feb 25, 2022

View reviewed changes

core/store/src/trie/trie_storage.rs Outdated Show resolved Hide resolved

mm-near reviewed Mar 7, 2022

View reviewed changes

remove default get_touched_nodes_count impl

ab55083

Merge branch 'master' into move-counter

09fd771

Longarithm self-assigned this Mar 9, 2022

Longarithm added the S-automerge label Mar 9, 2022

near-bulldozer bot merged commit f79a076 into master Mar 9, 2022

near-bulldozer bot deleted the move-counter branch March 9, 2022 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move nodes counter to the caching storage #6315

refactor: move nodes counter to the caching storage #6315

Longarithm commented Feb 17, 2022 •

edited

Loading

matklad Feb 17, 2022

Longarithm Feb 17, 2022

bowenwang1996 Feb 21, 2022

Longarithm Feb 22, 2022 •

edited

Loading

mzhangmzz Feb 23, 2022

Longarithm Feb 23, 2022

mzhangmzz Feb 24, 2022

Longarithm Feb 24, 2022 •

edited

Loading

bowenwang1996 left a comment

bowenwang1996 Feb 21, 2022

mzhangmzz left a comment

mm-near Mar 7, 2022

matklad Mar 15, 2022 •

edited

Loading

Longarithm commented Mar 9, 2022

refactor: move nodes counter to the caching storage #6315

refactor: move nodes counter to the caching storage #6315

Conversation

Longarithm commented Feb 17, 2022 • edited Loading

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Longarithm Feb 22, 2022 • 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

Longarithm Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

bowenwang1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzhangmzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Longarithm commented Mar 9, 2022

Longarithm commented Feb 17, 2022 •

edited

Loading

Longarithm Feb 22, 2022 •

edited

Loading

Longarithm Feb 24, 2022 •

edited

Loading

matklad Mar 15, 2022 •

edited

Loading