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

Costs-3 contract #3267

Merged
merged 46 commits into from
Nov 16, 2022
Merged

Costs-3 contract #3267

merged 46 commits into from
Nov 16, 2022

Conversation

pavitthrap
Copy link
Contributor

@pavitthrap pavitthrap commented Aug 29, 2022

Description

This PR adds new cost functions for the Clarity2 functions, and adds the costs-3 contract.

For the most part, the new costs contract does not alter old costs. However, as part of this benchmarking effort, I revamped the mocked data used (database and MARF accesses are now much more realistic). I also caught a few errors related to the computation of the input size (which only significantly altered one cost function). These changes led to a few cost changes. I provide a list of the altered costs below:

  • cost_stx_balance (increased 7x) - 99% of the runtime is in get_stx_balance_snapshot (DB access)
  • cost_stx_transfer (increased 7x) - 99% of the runtime is in get_stx_balance_snapshot (DB access)
  • cost_at_block (increased 10x) - using a real block now, which explains cost increase
  • cost_ast_parse (decreased 6x) (improvements in v2)
  • cost_concat (decreased 2x). (fixed input size computation)

We are now using version 0.21.0 of the Rust secp256k1 library. When we benchmarked the costs-2 contract, the version of this library was at 0.19.0. Due to improvements in these libraries, the runtimes for the following functions changed.

  • cost_secp256k1recover (decreased 2x)
  • cost_secp256k1verify (decreased 2x)
  • cost_poison_microblock (decreased 2x) - this function recovers the public key (so it essentially does the same operation as secp256k1recover)

Applicable issues

Additional info (benefits, drawbacks, caveats)

  • Not currently using new cost functions in the code base as that requires additional code change of loading the costs-3 contract (Load costs-3 contract  #3266) - I can also address that in this PR if people feel that is appropriate.

@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #3267 (4156983) into next (557f184) will decrease coverage by 0.03%.
The diff coverage is 19.47%.

@@            Coverage Diff             @@
##             next    #3267      +/-   ##
==========================================
- Coverage   29.60%   29.56%   -0.04%     
==========================================
  Files         279      284       +5     
  Lines      251259   258262    +7003     
==========================================
+ Hits        74378    76354    +1976     
- Misses     176881   181908    +5027     
Impacted Files Coverage Δ
clarity/src/vm/functions/assets.rs 26.96% <0.00%> (ø)
clarity/src/vm/functions/conversions.rs 14.56% <0.00%> (-0.40%) ⬇️
clarity/src/vm/functions/principals.rs 0.00% <0.00%> (ø)
clarity/src/vm/functions/sequences.rs 50.98% <0.00%> (-2.77%) ⬇️
clarity/src/vm/types/mod.rs 50.41% <0.00%> (-0.28%) ⬇️
src/burnchains/tests/affirmation.rs 0.00% <0.00%> (ø)
src/burnchains/tests/burnchain.rs 0.00% <0.00%> (ø)
src/burnchains/tests/db.rs 0.00% <0.00%> (ø)
src/burnchains/tests/mod.rs 0.00% <0.00%> (ø)
.../chainstate/burn/operations/leader_key_register.rs 15.39% <0.00%> (-0.03%) ⬇️
... and 92 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jcnelson
Copy link
Member

cost_stx_balance (increased 7x) - 99% of the runtime is in get_stx_balance_snapshot (DB access)
cost_stx_transfer (increased 7x) - 99% of the runtime is in get_stx_balance_snapshot (DB access)
cost_at_block (increased 10x) - using a real block now, which explains cost increase

Wow! 🤯 Do we know if there's a way to speed these operations up? Curious if this is due to a natural average MARF key-path lengthening that will naturally occur at a logarithmic scale as more data gets added, or if this is due to an artifact / regression of the reference implementation.

@jcnelson
Copy link
Member

Notes from the blockchain meeting:

  • This PR tests runtimes against a real DB, so the runtime cost includes the I/O costs of hitting a real MARF. Before, we had determined the read_count and write_count values with a real MARF, but determined the runtime values with a mocked MARF.

@kantai
Copy link
Member

kantai commented Sep 12, 2022

@pavitthrap raised a good point about the advantages of using the real backend versus the mocked backend -- this kind of relates to a question about the size of the MARF used in the benchmarks.

Do you know approximately how many keys were in the MARF, and how many MARF "blocks" there are in the database?

@pavitthrap
Copy link
Contributor Author

Update: will replace the MARF with a smaller one (up to 10 blocks).
Will re-benchmark the functions with the new MARF once the PR for the function replace-at is done.

Copy link
Contributor

@gregorycoppola gregorycoppola left a comment

Choose a reason for hiding this comment

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

Update: will replace the MARF with a smaller one (up to 10 blocks). Will re-benchmark the functions with the new MARF once the PR for the function replace-at is done.

Is this updated to use the mocked out MARF?

Is there a link to the data?

@jcnelson
Copy link
Member

Need a function for replace-at and the new analysis function for trait-checking

@jcnelson jcnelson self-assigned this Oct 17, 2022
Copy link
Contributor

@a3slade a3slade left a comment

Choose a reason for hiding this comment

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

LGTM

@igorsyl
Copy link
Contributor

igorsyl commented Oct 19, 2022

This PR tests runtimes against a real DB, so the runtime cost includes the I/O costs of hitting a real MARF. Before, we had determined the read_count and write_count values with a real MARF, but determined the runtime values with a mocked MARF.

@pavitthrap does the benchmark with real MARF use HDD or SSD?

@igorsyl
Copy link
Contributor

igorsyl commented Oct 19, 2022

  • It appears Load costs-3 contract  #3266 is needed for 2.1. Could we include it to this PR?
  • Do we have a programmatic check that the ClarityCostFunction enum is complete and not missing clarity functions?

@pavitthrap
Copy link
Contributor Author

This PR tests runtimes against a real DB, so the runtime cost includes the I/O costs of hitting a real MARF. Before, we had determined the read_count and write_count values with a real MARF, but determined the runtime values with a mocked MARF.

@pavitthrap does the benchmark with real MARF use HDD or SSD?

It uses a standard persistent disk, so HDD.

@jcnelson
Copy link
Member

jcnelson commented Nov 4, 2022

This should also address #3380 and #3381.

@pavitthrap
Copy link
Contributor Author

  • It appears Load costs-3 contract  #3266 is needed for 2.1. Could we include it to this PR?
  • Do we have a programmatic check that the ClarityCostFunction enum is complete and not missing clarity functions?

I'll be addressing #3380 and #3381, so I'll see about adding the contract when I do that.
I added a programmatic check now.

@jcnelson
Copy link
Member

jcnelson commented Nov 7, 2022

N.B. the benchmarks for I/O should use an SSD, because we don't want seeks and filesystem implementation details to interfere with benchmarks.

@jcnelson
Copy link
Member

jcnelson commented Nov 15, 2022

Okay @gregorycoppola @obycode @igorsyl @a3slade, I've made a few optimizations to this PR so that the cost functions for the stx-* functions are now only about 20% slower instead of over 8x slower. The reason for this is because we have to determine the current burnchain tip height in order to calculate the consolidated balance, which incurs some extra CPU cycles and I/O for loading up the data from the headers DB and parsing it out.

Once #3389 merges, I'll go and add cost functions for the bitwise operators.

This PR also addresses #3266. costs-3 is loaded as part of the epoch 2.1 instantiation.

In addition, this PR addresses #3381, #3380, and #3311.

@jcnelson
Copy link
Member

Okay, had to revert the implementation of ClarityDB::get_current_burnchain_block_height() because the new one was buggy. But, the other optimizations can remain in place. The stx-* methods are now 2x slower than they were in 2.05, due to the additional burnchain DB MARF query.

Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

LGTM

@igorsyl
Copy link
Contributor

igorsyl commented Nov 15, 2022

transition_empty_blocks test failing

@jcnelson jcnelson merged commit 66f2d13 into next Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants