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

fix(runtime costs): adjust runtime costs #2574

Merged
merged 4 commits into from
May 13, 2020
Merged

fix(runtime costs): adjust runtime costs #2574

merged 4 commits into from
May 13, 2020

Conversation

olonho
Copy link
Contributor

@olonho olonho commented Apr 29, 2020

Executed our runtime under emulator as described in
https://github.com/nearprotocol/nearcore/blob/master/runtime/runtime-params-estimator/emu-cost/README.md
to figure out the exact machine (x86 in our case) instruction counter required to perform basic operations.
Then normalized so that SHA256 computations cost aproximately the same.

Comparision with the old costs:
base: time=126224222 insn=88429085 insn/time=0.70
read_memory_base: time=1629369577 insn=773923915 insn/time=0.47
read_memory_byte: time=123816 insn=1266318 insn/time=10.23
write_memory_base: time=76445225 insn=95327640 insn/time=1.25
write_memory_byte: time=809907 insn=874985 insn/time=1.08
read_register_base: time=639340699 insn=743687111 insn/time=1.16
read_register_byte: time=63637 insn=31955 insn/time=0.50
write_register_base: time=0 insn=73022018 insn/time=n/a
write_register_byte: time=0 insn=7 insn/time=n/a
utf8_decoding_base: time=0 insn=162997218 insn/time=n/a
utf8_decoding_byte: time=591904 insn=96359 insn/time=0.16
utf16_decoding_base: time=0 insn=323612618 insn/time=n/a
utf16_decoding_byte: time=9095538 insn=6592872 insn/time=0.72
sha256_base: time=710092630 insn=651203831 insn/time=0.92
sha256_byte: time=5536829 insn=6626443 insn/time=1.20
keccak256_base: time=710092630 insn=1097138631 insn/time=1.55
keccak256_byte: time=5536829 insn=5700785 insn/time=1.03
keccak512_base: time=710092630 insn=1074127478 insn/time=1.51
keccak512_byte: time=5536829 insn=10762570 insn/time=1.94
log_base: time=0 insn=0 insn/time=n/a
log_byte: time=0 insn=0 insn/time=n/a
storage_write_base: time=21058769282 insn=13359081673 insn/time=0.63
storage_write_key_byte: time=23447086 insn=19416882 insn/time=0.83
storage_write_value_byte: time=9437547 insn=7084061 insn/time=0.75
storage_write_evicted_byte: time=0 insn=0 insn/time=n/a
storage_read_base: time=19352220621 insn=9743366720 insn/time=0.50
storage_read_key_byte: time=4792496 insn=7180693 insn/time=1.50
storage_read_value_byte: time=139743 insn=47159 insn/time=0.34
storage_remove_base: time=109578968621 insn=11051260769 insn/time=0.10
storage_remove_key_byte: time=9512022 insn=9337676 insn/time=0.98
storage_remove_ret_value_byte: time=0 insn=94568 insn/time=n/a
storage_has_key_base: time=20019912030 insn=9586751818 insn/time=0.48
storage_has_key_byte: time=4647597 insn=7175135 insn/time=1.54
storage_iter_create_prefix_base: time=28443562030 insn=0 insn/time=0.00
storage_iter_create_prefix_byte: time=442354 insn=0 insn/time=0.00
storage_iter_create_range_base: time=25804628282 insn=0 insn/time=0.00
storage_iter_create_from_byte: time=429608 insn=0 insn/time=0.00
storage_iter_create_to_byte: time=1302886 insn=0 insn/time=0.00
storage_iter_next_base: time=24213271567 insn=0 insn/time=0.00
storage_iter_next_key_byte: time=0 insn=0 insn/time=n/a
storage_iter_next_value_byte: time=1343211668 insn=0 insn/time=0.00
touching_trie_node: time=1 insn=1 insn/time=1.00
promise_and_base: time=0 insn=0 insn/time=n/a
promise_and_per_promise: time=672136 insn=541550 insn/time=0.81
promise_return: time=34854215 insn=97824365 insn/time=2.81

Test plan

cargo test

@gitpod-io
Copy link

gitpod-io bot commented Apr 29, 2020

Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov left a comment

Choose a reason for hiding this comment

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

Base storage access become almost twice cheaper, that's good news.

@MaksymZavershynskyi
Copy link
Contributor

log_base: time=0 insn=0 insn/time=n/a
log_byte: time=0 insn=0 insn/time=n/a

This is weird. With timing it is clear why this is near 0 seconds because it is almost instantaneous, but it should've accounted for some number of instructions..

Let's copy the values of utf8_decoding_base, utf8_decoding_byte here just to stay on the safe side.

@MaksymZavershynskyi
Copy link
Contributor

write_register_base: time=0 insn=73022018 insn/time=n/a
write_register_byte: time=0 insn=7 insn/time=n/a

This is great! Finally we have a tool sensitive enough to measure it.

Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

This is a change to the protocol. Please write a migration script and bump protocol version.

@MaksymZavershynskyi
Copy link
Contributor

@bowenwang1996 Could you provide code pointers to @olonho since he is new to the process?

@bowenwang1996
Copy link
Collaborator

Yeah. Follow the examples in scripts/migrates and write a script that changes config as you did in this PR. Also bump PROTOCOL_VERSION in core/chain-configs/src/lib.rs

@olonho
Copy link
Contributor Author

olonho commented Apr 30, 2020

Implemented migration tooling.

@MaksymZavershynskyi
Copy link
Contributor

@olonho RuntimeFeesConfig is still needed. @bowenwang1996 is it actually the right way to implement migration tool? AFAIU it consumes the old config and spits out the new one, so it should be also overwriting the values of the fees. Correct?

assert config['protocol_version'] == 10

config['protocol_version'] = 11

Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to overwrite the old values. See how it is done for example in migrate/scripts/6-state-stake.py

Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

@olonho I've fixed the migration script. Please let me know if I should merge this.

@olonho
Copy link
Contributor Author

olonho commented May 9, 2020

@olonho I've fixed the migration script. Please let me know if I should merge this.

Thanks a lot! Please don’t merge it so far, I have to decide with @nearmax if changes in #2621 makes sense, and then update costs with the new data.

@olonho
Copy link
Contributor Author

olonho commented May 11, 2020

Adjusted costs per new computatins.

@olonho olonho force-pushed the new_cost branch 2 times, most recently from 8e398e4 to d8c4e7c Compare May 11, 2020 11:47
"keccak256_byte": 21469644,
"keccak512_base": 5798128650,
"keccak512_byte": 36651981,
"log_base": 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like these were not updated using #2621 . Log should become non-zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, they were just hardcoded to be zeros, will fix.

"storage_iter_next_base": 0,
"storage_iter_next_key_byte": 0,
"storage_iter_next_value_byte": 0,
"touching_trie_node": 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make touching_trie_node temporarily equal to storage_read_base/log_16(20000) ?
Until we do certain optimizations to our trie this operation will have non-zero cost, so for the safety sake we could at least approximate it. The above formula would approximate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Both fixes added to #2621.

Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

Please address the two comments before merging. Thank you

Executed our runtime under emulator as described in
https://github.com/nearprotocol/nearcore/blob/master/runtime/runtime-params-estimator/emu-cost/README.md
to figure out the exact machine (x86 in our case) instruction counter required to perform basic operations.
Then normalized so that SHA256 computations cost aproximately the same and introduced 3x safety multiplier.

Comparision with the old costs:
base: time=126224222 insn=265287255 insn/time=2.10
read_memory_base: time=1629369577 insn=2321771745 insn/time=1.42
read_memory_byte: time=123816 insn=3798954 insn/time=30.68
write_memory_base: time=76445225 insn=285982920 insn/time=3.74
write_memory_byte: time=809907 insn=2624955 insn/time=3.24
read_register_base: time=639340699 insn=2231061333 insn/time=3.49
read_register_byte: time=63637 insn=95865 insn/time=1.51
write_register_base: time=0 insn=219066054 insn/time=n/a
write_register_byte: time=0 insn=21 insn/time=n/a
utf8_decoding_base: time=0 insn=488991654 insn/time=n/a
utf8_decoding_byte: time=591904 insn=289077 insn/time=0.49
utf16_decoding_base: time=0 insn=970837854 insn/time=n/a
utf16_decoding_byte: time=9095538 insn=19778616 insn/time=2.17
sha256_base: time=710092630 insn=1953611493 insn/time=2.75
sha256_byte: time=5536829 insn=19879329 insn/time=3.59
keccak256_base: time=710092630 insn=3291415893 insn/time=4.64
keccak256_byte: time=5536829 insn=17102355 insn/time=3.09
keccak512_base: time=710092630 insn=3222382434 insn/time=4.54
keccak512_byte: time=5536829 insn=32287710 insn/time=5.83
log_base: time=0 insn=0 insn/time=n/a
log_byte: time=0 insn=0 insn/time=n/a
storage_write_base: time=21058769282 insn=40077245019 insn/time=1.90
storage_write_key_byte: time=23447086 insn=58250646 insn/time=2.48
storage_write_value_byte: time=9437547 insn=21252183 insn/time=2.25
storage_write_evicted_byte: time=0 insn=0 insn/time=n/a
storage_read_base: time=19352220621 insn=29230100160 insn/time=1.51
storage_read_key_byte: time=4792496 insn=21542079 insn/time=4.49
storage_read_value_byte: time=139743 insn=141477 insn/time=1.01
storage_remove_base: time=109578968621 insn=33153782307 insn/time=0.30
storage_remove_key_byte: time=9512022 insn=28013028 insn/time=2.95
storage_remove_ret_value_byte: time=0 insn=283704 insn/time=n/a
storage_has_key_base: time=20019912030 insn=28760255454 insn/time=1.44
storage_has_key_byte: time=4647597 insn=21525405 insn/time=4.63
storage_iter_create_prefix_base: time=28443562030 insn=0 insn/time=0.00
storage_iter_create_prefix_byte: time=442354 insn=0 insn/time=0.00
storage_iter_create_range_base: time=25804628282 insn=0 insn/time=0.00
storage_iter_create_from_byte: time=429608 insn=0 insn/time=0.00
storage_iter_create_to_byte: time=1302886 insn=0 insn/time=0.00
storage_iter_next_base: time=24213271567 insn=0 insn/time=0.00
storage_iter_next_key_byte: time=0 insn=0 insn/time=n/a
storage_iter_next_value_byte: time=1343211668 insn=0 insn/time=0.00
touching_trie_node: time=1 insn=1 insn/time=1.00
promise_and_base: time=0 insn=0 insn/time=n/a
promise_and_per_promise: time=672136 insn=1624650 insn/time=2.42
promise_return: time=34854215 insn=293473095 insn/time=8.42

Test plan
---------
cargo test
@@ -7,4 +7,4 @@ pub use genesis_config::{
};

/// Current latest version of the protocol
pub const PROTOCOL_VERSION: u32 = 10;
pub const PROTOCOL_VERSION: u32 = 11;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bowenwang1996 we should make sure we don't collide with #2504. Just a heads up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We will not because the state migration check on CI will fail.

@olonho olonho force-pushed the new_cost branch 2 times, most recently from 05b65a5 to b8fd8f8 Compare May 13, 2020 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants