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

feat[codegen]: deallocate variables after last use #4219

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Sep 2, 2024

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

image

deallocate variables after last use, improving memory usage.
@charles-cooper charles-cooper changed the title feat[codegen]: deallocate variables after last-use feat[codegen]: deallocate variables after last use Sep 2, 2024
@charles-cooper charles-cooper added this to the v0.4.1 milestone Sep 2, 2024
@charles-cooper
Copy link
Member Author

charles-cooper commented Sep 2, 2024

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this accurate? we defer the dealoc until we hit its scope again (not necessarily its end)

Copy link
Collaborator

Choose a reason for hiding this comment

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

or if "it" refers to the scope then still we might have to wait until multiple scopes finish

Copy link
Member Author

Choose a reason for hiding this comment

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

"it" refers to the scope of the variable

Copy link
Collaborator

@cyberthirst cyberthirst Sep 17, 2024

Choose a reason for hiding this comment

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

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 deallocated

Copy link
Member Author

Choose a reason for hiding this comment

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

yea, this is the idea here

@@ -40,6 +40,7 @@ class VariableRecord:
defined_at: Any = None
is_internal: bool = False
alloca: Optional[Alloca] = None
system: bool = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

can't we rather modify the analysis than create a new type of variable given it's used only at one place?

Copy link
Member Author

Choose a reason for hiding this comment

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

yea, it's a bit of a kludge, until we refactor sqrt to be either pure IR or a library function

@cyberthirst
Copy link
Collaborator

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".

@charles-cooper
Copy link
Member Author

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

@charles-cooper
Copy link
Member Author

closing and reopening to try to fix the codecov report

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 95.65217% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.14%. Comparing base (dd5a3d9) to head (ab71f77).

Files with missing lines Patch % Lines
vyper/codegen/context.py 89.47% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4219      +/-   ##
==========================================
+ Coverage   92.06%   92.14%   +0.07%     
==========================================
  Files         120      120              
  Lines       17335    17368      +33     
  Branches     2935     2943       +8     
==========================================
+ Hits        15960    16003      +43     
+ Misses        957      951       -6     
+ Partials      418      414       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants