feat[codegen]: deallocate variables after last use#4219
feat[codegen]: deallocate variables after last use#4219charles-cooper wants to merge 18 commits intovyperlang:masterfrom
Conversation
deallocate variables after last use, improving memory usage.
charles-cooper
changed the title
|
marked this for inclusion in 0.4.1, but depending on when our next audit can be, might push to 0.4.2 |
use_count inside of Expr is dangerous, since Expr() can be called multiple times for a given node. separate the analysis.
| var = self.vars[varname] | ||
| for s in self._scopes: | ||
| if s not in var.blockscopes: | ||
| # defer deallocation until we hit the end of its scope |
There was a problem hiding this comment.
is this accurate? we defer the dealoc until we hit its scope again (not necessarily its end)
There was a problem hiding this comment.
or if "it" refers to the scope then still we might have to wait until multiple scopes finish
There was a problem hiding this comment.
"it" refers to the scope of the variable
There was a problem hiding this comment.
but we sweep after statements, right?
a: uint256 = 0
for i: uint256 in range(1, 3):
a += i # <- can't dealoc, defer
b: uint256 = 0 # a already deallocated, swept after the for
b += 1
return b # actual scope end but a is already deallocatedThere was a problem hiding this comment.
yea, this is the idea here
|
Given that we log write as both write+read, the dealoc won't happen until the last write (although the memory variable might be long dead at that point). So, in this context, the write operation is considered as "use". |
i think a write is a use |
|
closing and reopening to try to fix the codecov report |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4219 +/- ##
==========================================
+ Coverage 92.16% 92.24% +0.07%
==========================================
Files 122 122
Lines 17471 17504 +33
Branches 2959 2967 +8
==========================================
+ Hits 16103 16146 +43
+ Misses 957 951 -6
+ Partials 411 407 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| last_use = self.expr._metadata.get("last_use", False) | ||
| if last_use and var.location == MEMORY: | ||
| self.context.mark_for_deallocation(varname) |
There was a problem hiding this comment.
what if the variable is entirely unused? then we won't have the use to deallocate it
so that we don't have to deal with side-effects? |
|
reminder to run this against https://github.com/pcaversaccio/snekmate gas snapshots to consider whether the inclusion of this PR is worth it |
charles-cooper
added
the
release - tentative
| if varname in self.forvars: | ||
| return | ||
| if self.vars[varname].system: | ||
| return |
There was a problem hiding this comment.
As far as I can determine, this line cannot be covered because builtins such as sqrt are not analyzed for last use.
Also a comment for why these variables are not marked for deallocation (such as the one for forvars) would be nice.
| info = expr._expr_info | ||
| if info is None: | ||
| continue | ||
| for r in info._reads: |
There was a problem hiding this comment.
Not asking for a change, but I found it a bit confusing that _reads also include writes.
| @@ -182,8 +182,14 @@ def parse_Name(self): | |||
|
|
|||
| # local variable | |||
| if varname in self.context.vars: | |||
There was a problem hiding this comment.
Not really related to this PR, but self.context.vars actually contains varnames which than have to be mapped to actual vars. Perhaps it should be renamed to reflect this (as in self.context.varnames) in the future.
|
obsolesced by #4756 |
4 participants
deallocate variables after last use, improving memory usage.
What I did
How I did it
How to verify it
Commit message
Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)
Description for the changelog
Cute Animal Picture