-
-
Notifications
You must be signed in to change notification settings - Fork 788
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
Assign storage locations prior to LLL gen / place arrays sequentially in storage #2361
Assign storage locations prior to LLL gen / place arrays sequentially in storage #2361
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.
Overall, I think this represents a breaking change to how our storage allocation algorithm works, and should perhaps target a v0.3.0 release
Could do with more robust testing, but we only have Bytes
and String
in terms of dynamic types right now.
@@ -175,6 +175,7 @@ def generate_folded_ast( | |||
vy_ast.folding.fold(vyper_module_folded) | |||
validate_semantics(vyper_module_folded, interface_codes) | |||
vy_ast.expansion.expand_annotated_ast(vyper_module_folded) | |||
set_data_positions(vyper_module_folded) |
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.
It would be nice if the storage and memory layout generated during this phase were made available externally
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.
Yeah, makes sense as a compiler output. But I think better done after the entirety of data positions are being calculated in that pass.
cbcdeed
to
13e8ae8
Compare
Codecov Report
@@ Coverage Diff @@
## master #2361 +/- ##
==========================================
- Coverage 85.90% 85.80% -0.11%
==========================================
Files 90 91 +1
Lines 8940 8986 +46
Branches 2139 2142 +3
==========================================
+ Hits 7680 7710 +30
- Misses 771 783 +12
- Partials 489 493 +4
Continue to review full report at Codecov.
|
I don't think this requires a bump to
|
Should at least require a shoutout in release notes |
This commit only includes logic for determining storage slots, but also lays out an API to add in logic for memory and calldata positions.
13e8ae8
to
5ed060f
Compare
What I did
How I did it
Currently, whenever dealing with a type that requires >1 storage slot, we calculate the storage location in the same way as that of a mapping (using a keccak). This is inefficient for both gas costs and bytecode size. Because Vyper does not support dynamically-sized variables without a limit on their length, it is possible to calculate the maximum required number of slots for all storage variables and instead place them sequentially to avoid the expensive
SHA3
operation.To do this, I've added the
DataPosition
class, and related child classes, to represent positions within storage/memory/calldata. After type checking is completed, the AST is parsed once more and these objects applied to each previously assigned type. Handling this as a separate pass is imo a cleaner approach, making it easier to understand where and how this happens.Armed with this information and the new storage layout, I've then removed various references to
sha_32
withinparser
. I had to do a couple hacky things to make the new play well with the old, but mostly it was a reductive operation so I think this is a net win in terms of the bigger refactor goal.I have more logic ready to go re: assigning locations for memory and calldata, but for the scope of this PR all I've included is the eventual API to show my idea for the bigger picture without doing too much in a single chunk.
How to verify it
Run the tests. I added a lovely (awful?) new test case that assigns to and reads from a variety of storage slots to check for corruption. I also ran the Curve test suite against this PR, everything passes.
Cute Animal Picture