-
Notifications
You must be signed in to change notification settings - Fork 131
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
String optimization and addition of Suffix() #126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Minor comment about the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, but I have some suggestions for further improvements. Specifically, there are 2 cases where the current implementation behaves sub-optimally:
-
Substring(Bytes("..."), Int(255), Int(257))
currently translates tobyte "..."; int 255; int 257; substring3
. If theextract
opcode is available, it would be more efficient to translate this tobyte "..."; extract 255 2
-
Suffix(Bytes("..."), Int(256))
currently translates tobyte "..."; int 256; byte "..."; len; substring 3
. If thedig
opcode is available, it would be more efficient & correct to translate this tobyte "..."; int 256; dig 1; len; substring 3
(the dig opcode copies a value that's already on the stack).- It would be more efficient because the input string might be the result of a computation (e.g.
Concat(Bytes("a"), Bytes("b"), Bytes("c"))
), so we should avoid doing the computation twice. - Additionally, it would be more correct because the input string can be the result of a computation that has other sides effects, like
Seq(Log(Bytes("hello")), Bytes("myString"))
. So duplicating the computation would add unintended behavior.- You could also add a workaround using other opcodes in case someone is compiling a program for TEAL v2 where
dig
is not available, but I think it would be fine for this expression to just raise an error in that case.
- You could also add a workaround using other opcodes in case someone is compiling a program for TEAL v2 where
- It would be more efficient because the input string might be the result of a computation (e.g.
I have some suggestions for how to address the above issues. The first, mainly for organization, is to create a new file, something like pyteal/ast/substring.py
, and move all substring, extract, and suffix expressions into there, to keep ternaryexpr.py
simple.
The second would be to turn the existing Substring
, Extract
, and Suffix
functions into full classes which extend the Expr
base class. These classes would essentially do the same inspection of arguments that you've already implemented, but they should happen inside the __teal__
method when the AST is being compiled, rather than when the AST is being created. This is beneficial because in the __teal__
method, you'll have access to the current TEAL version that's being targeted during compilation, so you can choose the most efficient opcodes for the given arguments and TEAL version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very good! I have a few minor comments, the most important of which is the case where l = 0
in SubstringExpr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
This reverts commit 7cb7b9a.
This reverts commit 564e602.
This reverts commit 7cb7b9a.
* Optimization added for repeated int constants under 2**7 w/ tests * fixed type problem and formatted * Expanded test and added comment for clarification * implement optimization utility with simple slot store/load canceling * minor refactor * reformat code * Update pyteal/compiler/optimizer/optimizer.py Co-authored-by: Michael Diamant <[email protected]> * Update pyteal/compiler/optimizer/optimizer.py Co-authored-by: Michael Diamant <[email protected]> * Adding exponentiation to arithmatic ops docs (#134) Add missing exponentiation operation in document * updating to use new syntax for seq (#135) * updating to use new syntax for seq * rewording * Make pylance recognize wildcard imports (#133) * 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 * Include pyi files in build (#137) * Revert "Optimization for constant assembly (#128)" This reverts commit 5636ccd. * Revert "String optimization and addition of Suffix() (#126)" This reverts commit 7cb7b9a. * Update to v0.9.1 (#138) * Revert "Revert "String optimization and addition of Suffix() (#126)"" This reverts commit 564e602. * Revert "Revert "Optimization for constant assembly (#128)"" This reverts commit cc405a5. * Update examples.rst (#140) * Fix type for App.globalGetEx in docs (#142) * up max teal version (#146) * up max teal version * make test fail if its greater than version defined as MAX_TEAL_VERSION * Fmt * hardcode to 7 * Add version 6 test * Formatting subroutines with name and newline (#148) * using the subroutine name for the label * adding newline before label declaration, fix tests to account for newline * remove commented name, fix test * only add newline for subroutines with comment * naming with suffix * adding test for invalid name * Call type_of() in require_type() for better exception messages (#151) * call type_of in require_type to catch exceptions * fix formatting for types.py and types_test.py * `method` pseudo-op support for ABI methods (#153) - Add support for `method` pseudo-opcode in PyTeal. - Add `name` field in `subroutine` to override __name__ from function implementation, for readability in generated code. * Print diff of `__init__.pyi` (#166) * Print diff of __init__.pyi * Format * Undo travis change * C2C Feature Support (#149) - `itxn_next` implementation / test - `itxn_field` support for array field setting - `gitxn / gitxna` implementation / test - `gloadss` implementation / test * Add BytesSqrt (#163) * Add BytesSqrt * Update pyteal/ast/unaryexpr_test.py Co-authored-by: Jason Paulos <[email protected]> Co-authored-by: Jason Paulos <[email protected]> * adding new globals from teal6 (#168) * adding new globals from teal6 * fmt * Acct params get (#165) * Adding account param getter * Add to init * fix op names and type * adding tests * allow bytes to be passed * tweak docs, add require check for any * Change Subroutine Wrapped Callable to a class with call method (#171) Allows for more information (name, return type, has return) about the subroutine extractable from wrapped fnImpl by subroutine * Subroutine Type Annotations (#182) This PR requires that any type annotation of a Subroutine parameter or return value be of type `Expr`. Missing annotations are assumed to be `Expr`'s as well. In a follow up PR #183 this restriction will be loosened. * fix docs referencing what apps should eval to (#191) * Move from Travis to Github Actions (#190) * MultiValue expression implemented to support opcodes that return multiple values (#196) * Optimization added for repeated int constants under 2**7 w/ tests * fixed type problem and formatted * Expanded test and added comment for clarification * add multivalue expr and change maybevalue to derive from multivalue * updated tests and formatting * reorder output slots to reflect stack ordering * add additional assertion in MaybeValue test to enforce slot ordering * Support TEAL 6 txn fields LastLog, StateProofPK and opcodes divw, itxnas, gitxnas (#174) * adding new teal6 ops, no pyteal expressions defined for them yet * Add opcode support for divw * Add opcode support for divw (#192) * Add opcode support for itxnas and gitxnas (#193) * Add opcode support for itxnas and gitxnas * Update stale reference to inner transaction limit * Fix allowed types for GitxnaExpr txnIndex * Remove obsolete logic for handling GitxnaExpr.teal construction * Remove unnecessary cast and fix gitxna runtime type checking * Move type validation to constructors for gtxn and gitxn variants * Add missed tests from prior commit * Fix duplicate test case * Move index validation from subclasses to TxnaExpr * Inline validation functions per PR feedback * Remove unused imports * Refactor to isinstance tupled check * Remove TEAL v1 min version test per PR feedback * Fix constructor type checking for GtxnExpr * Refactor to remove duplicate type check function * Update last_log docstring Co-authored-by: Jason Paulos <[email protected]> * Expose state_proof_pk txn field * Update transaction field docs to reflect TEAL v6 * Update transaction field docs to reflect TEAL v6 Co-authored-by: michaeldiamant <[email protected]> Co-authored-by: Jason Paulos <[email protected]> * Fixed typo (#202) * Add Github action to generate docset (#201) * Add build docset step * non-slim container * Update docs to group transaction field tables like go-algorand (#204) * Update accessing_transaction_field.rst to fix typo (#207) * Add docs README to explain docs/ testing procedure (#205) * v0.10.0 (#206) * Update to v0.10.0 * Add latest commits to changelog * fixing github actions to run on tags (#208) * Update build.yml * Update build.yml * Fix typos in docstrings and error messages (#211) * Test on Python 3.10 (#212) * Update versions.rst (#210) * Update versions.rst content of [https://github.com/algorand/pyteal/releases] is not shown in [https://pyteal.readthedocs.io/en/latest/versions.html] * Update docs/versions.rst Co-authored-by: Jason Paulos <[email protected]> Co-authored-by: Jason Paulos <[email protected]> * Pass-by-Ref / Dynamic Scratch Variables via the `loads` and `stores` opcodes (#198) * Pass-by-Reference Semantics * Use a Dynamic ScratchVar to "iterate" over other ScratchVar's * Another approach for E2E testing * Fix build script invocation (#223) * Bring #225 to master (#227) * Ignore tests generating TEAL file outputs used for expected comparisons (#228) * Fix typo in CONTRIBUTING.md (#229) * Fix subroutine mutual recursion with different argument counts bug (#234) * Fix mutual recursion bug * Remove usage of set.pop * Revert "Pass-by-Ref / Dynamic Scratch Variables via the `loads` and `stores` opcodes (#198)" This reverts commit cf95165. * v0.10.1 (#237) * Revert "Revert "Pass-by-Ref / Dynamic Scratch Variables via the `loads` and `stores` opcodes (#198)"" This reverts commit 51ec8c9. * Update user guide docs to reflect addition of DynamicScratchVar (#226) * Update CONTRIBUTING.md on PEP 8 naming conventions policy (#241) * implement optimization utility with simple slot store/load canceling * minor refactor * reformat code * correct import format to match convention * slot optimization awareness of reserved ids added * fix typo * remove dataclass usage * slight reorg of compiler process in order to perform optimization on cfg * clean up imports * updated documentation and reformatted with new version of black * remove unused imports and comments * reformatting * add additional optimizer unit tests * improve testing and slight refactoring * more renaming * documentation and import changes * fixed typos in docs Co-authored-by: Michael Diamant <[email protected]> Co-authored-by: Ben Guidarelli <[email protected]> Co-authored-by: Jason Paulos <[email protected]> Co-authored-by: Edward D Gaudio <[email protected]> Co-authored-by: Joe Polny <[email protected]> Co-authored-by: Hang Su <[email protected]> Co-authored-by: Łukasz Ptak <[email protected]> Co-authored-by: Zeph Grunschlag <[email protected]> Co-authored-by: Jack <[email protected]> Co-authored-by: Glory Agatevure <[email protected]> Co-authored-by: Adriano Di Luzio <[email protected]> Co-authored-by: PabloLION <[email protected]>
Optimization of Substring() and Extract() functions when int args are both immediate and fit in one byte. This is achieved by utilizing the substring and extract opcodes rather than substring3 and extract3.