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

AVM: Rework around opcode fields for more flexible costs #3832

Merged
merged 9 commits into from
Mar 28, 2022

Conversation

jannotti
Copy link
Contributor

This PR makes is possible for opcodes to have different costs depending on their immediate argument. It also opens the door to opcodes that have different costs based on their run-time arguments (on stack).

There was significant rework around how information about opcode fields (immediates) is stored. This rework:

  1. Makes doc generation more automatic
  2. Eliminates the need for custom opcode disassemblers
  3. Eliminates one hashmap lookup per evaluation of opcodes with immediate fields.
  4. Places more documentation "inline" with definitions - immediates for opcodes, and docs for fieldspecs.

Simplifies adding a new field. Eliminates need for custom
disassembler. Avoids hash lookup in opcode evaluation. Simplifies
opdoc.
@jannotti jannotti changed the title Rework around opcode fields for more flexible costs AVM: Rework around opcode fields for more flexible costs Mar 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2022

Codecov Report

Merging #3832 (8256400) into master (bc19540) will decrease coverage by 0.01%.
The diff coverage is 72.75%.

@@            Coverage Diff             @@
##           master    #3832      +/-   ##
==========================================
- Coverage   49.98%   49.96%   -0.02%     
==========================================
  Files         392      392              
  Lines       68501    68386     -115     
==========================================
- Hits        34239    34169      -70     
+ Misses      30501    30475      -26     
+ Partials     3761     3742      -19     
Impacted Files Coverage Δ
data/transactions/logic/fields_string.go 40.00% <ø> (ø)
data/transactions/logic/doc.go 45.45% <20.00%> (-10.55%) ⬇️
data/transactions/logic/fields.go 46.80% <59.04%> (-2.43%) ⬇️
data/transactions/logic/eval.go 89.60% <83.78%> (-0.08%) ⬇️
data/transactions/logic/assembler.go 81.33% <83.96%> (+1.37%) ⬆️
data/transactions/logic/opcodes.go 97.05% <90.47%> (-2.95%) ⬇️
agreement/cryptoVerifier.go 68.08% <0.00%> (-2.13%) ⬇️
agreement/proposalManager.go 96.07% <0.00%> (-1.97%) ⬇️
data/transactions/verify/txn.go 44.15% <0.00%> (-0.87%) ⬇️
network/wsNetwork.go 62.79% <0.00%> (-0.20%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc19540...8256400. Read the comment docs.

Copy link
Contributor

@michaeldiamant michaeldiamant left a comment

Choose a reason for hiding this comment

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

@jannotti Thanks for finding a way to support variable field costs while also simplifying opcode maintenance. ☕

@jannotti jannotti merged commit 7126597 into algorand:master Mar 28, 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.

4 participants