AVM: frame pointers by jannotti · Pull Request #4319 · algorand/go-algorand

jannotti · 2022-07-28T17:44:57Z

No description provided.

ahangsu

took a quick pass, looks like interesting fine grained manipulation over stack. I think the subroutine scheme in Pyteal will benefit from it, by getting rid of allocating scratch slots (plausibly trivially).

tzaffi

Looking very good.

Still reading, but I wanted to provide some feedback at this juncture.

codecov · 2022-08-30T21:23:10Z

Codecov Report

Merging #4319 (74fa7eb) into master (d3c10f4) will increase coverage by 0.11%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           master    #4319      +/-   ##
==========================================
+ Coverage   54.09%   54.21%   +0.11%     
==========================================
  Files         401      402       +1     
  Lines       51659    51830     +171     
==========================================
+ Hits        27947    28101     +154     
- Misses      21355    21364       +9     
- Partials     2357     2365       +8

Impacted Files	Coverage Δ
data/transactions/logic/doc.go	`61.53% <ø> (ø)`
data/transactions/logic/assembler.go	`85.39% <82.55%> (-0.58%)`	⬇️
data/transactions/logic/debugger.go	`61.46% <100.00%> (ø)`
data/transactions/logic/eval.go	`89.40% <100.00%> (+0.09%)`	⬆️
data/transactions/logic/frames.go	`100.00% <100.00%> (ø)`
data/transactions/logic/opcodes.go	`84.56% <100.00%> (+0.09%)`	⬆️
ledger/roundlru.go	`90.56% <0.00%> (-5.67%)`	⬇️
cmd/algoh/blockWatcher.go	`77.77% <0.00%> (-3.18%)`	⬇️
ledger/blockqueue.go	`84.48% <0.00%> (-1.15%)`	⬇️
cmd/tealdbg/debugger.go	`72.69% <0.00%> (-0.81%)`	⬇️
... and 7 more

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

tzaffi

Very useful functionality. Very much looking forward to seeing how PyTeal subroutines become sleeker and easier to handle thanks to all this.

I left several comments, some more nitpicky than others.

My biggest suggestion is that a solid e2e test for the functionality be included before a final merge.

tzaffi

Good juncture to leave comments. Still reading

tzaffi

Submitting a couple of more questions.

Also, a lot of cleanup of type tracking with frame_* And, caught some examples of us not testing opcodes in "nonsense" tests because the opcode was a substring of another opcode, like & is a substring of &&. (Caught, because "bury" is a substring of "frame_bury") This means there is a change to the "old" compiled constants. This is normally not done, but here we added to an old nonsense test, we added the corresponding bytes to the compiled output.

jasonpaulos

Except for the possibility of increasing costs for dupn or proto (maybe that investigation ought to be a separate issue?), this looks good to me

tzaffi

I expect to add some more question in the afternoon, but might as well put some up now.

tzaffi

All of the items that I have previously brought up have been considered.

jannotti mentioned this pull request Jul 28, 2022

add frame pointer opcodes #4304

Closed

jannotti force-pushed the avm-frame-pointers branch from 9ee23c4 to 0822b24 Compare July 28, 2022 19:50

ahangsu reviewed Aug 2, 2022

View reviewed changes

Comment thread data/transactions/logic/frames.go Outdated

Comment thread data/transactions/logic/frames.go Outdated

Comment thread data/transactions/logic/eval.go Outdated

Comment thread data/transactions/logic/eval.go Outdated

jannotti force-pushed the avm-frame-pointers branch from 0822b24 to ec9f161 Compare August 2, 2022 18:31

jannotti added the New Feature label Aug 2, 2022

jannotti force-pushed the avm-frame-pointers branch 3 times, most recently from 969600e to ab8ed01 Compare August 7, 2022 16:47

ahangsu reviewed Aug 8, 2022

View reviewed changes

Comment thread data/transactions/logic/doc.go Outdated

tzaffi reviewed Aug 26, 2022

View reviewed changes

Comment thread data/transactions/logic/frames_test.go Outdated

ahangsu reviewed Aug 26, 2022

View reviewed changes

Comment thread data/transactions/logic/eval.go Outdated

jannotti force-pushed the avm-frame-pointers branch 2 times, most recently from 144bcc6 to d7b245f Compare August 29, 2022 19:05

tzaffi reviewed Aug 30, 2022

View reviewed changes

Comment thread data/transactions/logic/frames_test.go Outdated

Comment thread data/transactions/logic/frames_test.go Outdated

Comment thread data/transactions/logic/doc.go Outdated

Comment thread data/transactions/logic/eval.go Outdated

michaeldiamant reviewed Aug 30, 2022

View reviewed changes

Comment thread data/transactions/logic/eval.go Outdated

tzaffi reviewed Aug 31, 2022

View reviewed changes

Comment thread data/transactions/logic/eval_test.go Outdated

tzaffi reviewed Sep 1, 2022

View reviewed changes

Comment thread data/transactions/logic/frames.go Outdated

ahangsu reviewed Sep 15, 2022

View reviewed changes

Comment thread data/transactions/logic/TEAL_opcodes.md Outdated

tzaffi reviewed Sep 16, 2022

View reviewed changes

Comment thread data/transactions/logic/opcodes.go Outdated

Comment thread data/transactions/logic/doc.go Outdated

Comment thread data/transactions/logic/assembler_test.go Outdated

ahangsu reviewed Sep 16, 2022

View reviewed changes

Comment thread data/transactions/logic/assembler.go Outdated

tzaffi reviewed Sep 16, 2022

View reviewed changes

Comment thread data/transactions/logic/frames.go Outdated

Comment thread data/transactions/logic/frames_test.go Outdated

jannotti force-pushed the avm-frame-pointers branch 2 times, most recently from 2ea3c34 to 3d4c884 Compare September 19, 2022 03:00

michaeldiamant mentioned this pull request Sep 19, 2022

AVM 8: Opcode support algorand/pyteal#388

Closed

2 tasks

ahangsu reviewed Sep 19, 2022

View reviewed changes

Comment thread data/transactions/logic/assembler.go Outdated

ahangsu reviewed Sep 19, 2022

View reviewed changes

Comment thread data/transactions/logic/assembler.go

ahangsu reviewed Sep 19, 2022

View reviewed changes

Comment thread data/transactions/logic/assembler.go

ahangsu reviewed Sep 19, 2022

View reviewed changes

Comment thread data/transactions/logic/assembler.go

jannotti and others added 8 commits September 27, 2022 13:24

Smarter limits on frame_{dig,bury}

2111bd7

clearer error message

18a7933

Intial DupN op.

184313e

Remove stacky() in favor of typed() + immediates()

e22fabc

Make bury and frame_bury type checking look more similar

d803ad3

Fix small bug and other improvements

badafe1

Excised pushn

5937ea6

jannotti force-pushed the avm-frame-pointers branch from aac5193 to 5937ea6 Compare September 27, 2022 17:25

jannotti added 2 commits September 27, 2022 14:12

Spec detail for retsub from a proto frame.

aac2007

Re-org the new opcodes, move frames out of experimental

21ff73b

jannotti force-pushed the avm-frame-pointers branch from 8ee62a7 to 21ff73b Compare September 27, 2022 18:38

jasonpaulos previously approved these changes Sep 27, 2022

View reviewed changes

jannotti dismissed jasonpaulos’s stale review via b213777 September 28, 2022 14:45

remove a stray "b" in docs

b58ff8e

jannotti force-pushed the avm-frame-pointers branch from b213777 to b58ff8e Compare September 28, 2022 14:45

tzaffi reviewed Sep 28, 2022

View reviewed changes

Comment thread data/transactions/logic/opcodes.go Outdated

Comment thread data/transactions/logic/opcodes.go Outdated

Comment thread data/transactions/logic/opcodes.go

Remove useless clones

41dae52

tzaffi reviewed Sep 28, 2022

View reviewed changes

Comment thread data/transactions/logic/frames.go

Avoid allocation, and iterating twice, in dupn

1bc1de7

tzaffi previously approved these changes Sep 28, 2022

View reviewed changes

Coverage, small optimization

74fa7eb

jannotti dismissed tzaffi’s stale review via 74fa7eb September 28, 2022 18:09

tzaffi approved these changes Sep 28, 2022

View reviewed changes

jannotti merged commit 5be155b into algorand:master Sep 28, 2022

michaeldiamant mentioned this pull request Sep 29, 2022

Implement subroutine arguments and return values using frame pointers algorand/pyteal#547

Closed

This was referenced Sep 30, 2022

go-algorand 3.10.0-beta Release PR #4612

Merged

go-algorand 3.10.0-stable Release PR #4618

Merged

This was referenced Oct 13, 2022

Support local variables with frame pointers algorand/pyteal#559

Closed

Support MultiValue + looping via frame pointers algorand/pyteal#560

Open

Conversation

jannotti commented Jul 28, 2022

Uh oh!

ahangsu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tzaffi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

tzaffi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tzaffi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tzaffi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasonpaulos left a comment

Choose a reason for hiding this comment

Uh oh!

tzaffi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tzaffi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ahangsu left a comment •

edited

Loading

codecov Bot commented Aug 30, 2022 •

edited

Loading