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

Tools : PyTeal optimization utility #247

Merged
merged 75 commits into from
Mar 31, 2022

Conversation

algoidurovic
Copy link
Contributor

@algoidurovic algoidurovic commented Mar 16, 2022

This PR implements the first iteration of the PyTeal optimization utility. The first optimization to be included is the removal of contiguous store/load opcodes that are deemed redundant.

Background:
PyTeal often uses ScratchSlots implicitly to bind sections of the source code together. This pattern is best illustrated by the implementations of PyTeal subroutines and MultiValue expressions. The issue arises at the TEAL level when ScratchSlots are utilized naively, leading to redundant store/load opcodes (ie the ops cancel each other out) that can be removed without affecting the behavior of the program.

The conditions for applying this optimization are very simple -- and perhaps unnecessarily narrow:

  1. candidate for removal must match the pattern [store i, load i] where i is the index of a scratch slot.
  2. the same scratch slot cannot be loaded from anywhere else in the code.

Testing:
The unit tests cover both the optimization of hardcoded low level teal and the optimization of teal generated by the compiler from a PyTeal program. Tests cover subroutines, MultiValue expressions, ScratchVars, and more.

In order to gauge the practical utility of this optimization, I applied it to the Algorand Auction Demo. Unfortunately, the scope of the optimization was too narrow to apply to the demo. Further investigation (market research?) is needed here to determine a fruitful direction to take the optimizer in the future.

@@ -1,4 +1,7 @@
from typing import List, Tuple, Set, Dict, Optional, cast
from pyteal.compiler import optimizer
Copy link
Contributor

Choose a reason for hiding this comment

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

@algoidurovic Unrelated to the line: Since we're mostly agreed on the optimization to-be-provided, can you update the user guide?

Nominally, https://pyteal.readthedocs.io/ ought to explain how users can opt-in for optimization.

@algoidurovic algoidurovic changed the title [WIP] Implement optimization utility Tools : PyTeal optimization utility Mar 22, 2022
algoidurovic and others added 11 commits March 23, 2022 13:12
Add missing exponentiation operation in document
* updating to use new syntax for seq

* rewording
* adding exports directly to top level __all__

* apply black formatter

* adding initial generate script

* fmt

* rm all from all

* adding check to travis

* reading in original __init__ and using its imports, dont write to filesystem if --check is passed

* make messages more profesh

* fix flags after black formatted them

* y

* flippin black formatter

* help text fix

* asdfasdf
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

I have one concern that must be addressed before releasing this:

  1. The optimization looks correct except for the case where a user makes a ScratchVar with a specifically requested slot ID. In that case, we need to respect that slot ID and not optimize the scratch slot away, so this optimization should likely not touch any ScratchSlot with a user-defined ID. Based on the current code, this is not checked.
    • Item 1 from the next list would be one way to solve this, since if you operate on ScratchSlot objects you can tell if they're user-defined or not.

And a few that need not be addressed now, but should be before more optimization work takes place. We can talk about these in more detail next week if you'd like.

  1. This optimization happens after ScratchSlot objects have been assigned integer IDs. This step of the compilation will fail if more than 256 scratch slots are in use, so it would be more useful if this optimization can be applied before that happens. I realize the flow of the compiler as it is currently doesn't make this easy, but it should be possible to reorder compilation events to make this happen.
  2. Currently this optimization happens on the linearized list of TealComponents. For this simple optimization, that's acceptable, but anything more complex will need access to more control flow information than is available at this stage and should probably operate with the control flow graph. For that reason I'd recommend changing this optimization to also operate on TealBlocks before we implement more advanced features.

Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

Just some minor comments here and there after a second pass on implementation

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for addressing all concerns!

@algoidurovic algoidurovic merged commit 2e59bbf into algorand:master Mar 31, 2022
@jasonpaulos jasonpaulos mentioned this pull request Apr 15, 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.