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

Vine: Memory Leak Supplement #3598

Merged
merged 2 commits into from
Jan 9, 2024
Merged

Vine: Memory Leak Supplement #3598

merged 2 commits into from
Jan 9, 2024

Conversation

JinZhou5042
Copy link
Member

@JinZhou5042 JinZhou5042 commented Jan 9, 2024

Proposed changes

This PR is a supplement of #3592.

The memory leak issue of FunctionCall tasks manifests on two sites:

  • Python maintains references to task objects, failing to timely delete them.
  • The manager's declared input and output buffers were not properly disposed of immediately after output retrieval.

While #3592 resolved the first issue, this PR remedies the second.

Comparison of consumed memory before and after changes in this PR with an example executed on 100 tasks:

Screenshot from 2024-01-09 11-53-05

Screenshot from 2024-01-09 11-51-30

Post-change actions

Put an 'x' in the boxes that describe post-change actions that you have done.
The more 'x' ticked, the faster your changes are accepted by maintainers.

  • make test Run local tests prior to pushing.
  • make format Format source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)
  • make lint Run lint on source code prior to pushing.
  • Manual Update Did you update the manual to reflect your changes, if appropriate? This action should be done after your changes are approved but not merged.
  • Type Labels Select github labels for the type of this change: bug, enhancement, etc.
  • Product Labels Select github labels for the product affected: TaskVine, Makeflow, etc.
  • PR RTM Mark your PR as ready to merge.

Additional comments

This section is dedicated to changes that are ambitious or complex and require substantial discussions. Feel free to start the ball rolling.

@JinZhou5042 JinZhou5042 requested a review from btovar January 9, 2024 16:56
@JinZhou5042 JinZhou5042 added TaskVine bug For modifications that fix a flaw in the code. labels Jan 9, 2024
Copy link
Member

@dthain dthain left a comment

Choose a reason for hiding this comment

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

This looks like the right approach.

While a user would normally call output on a successful task, there are some circumstances where they might not. (Maybe the task failed.)

Suggest that you set _input and _output_buffer to none when they are removed.

And then do a final check (and removal if needed) when the object is deleted.

@JinZhou5042
Copy link
Member Author

Those are done.

@JinZhou5042 JinZhou5042 requested a review from dthain January 9, 2024 18:52
@dthain dthain merged commit 2df7c68 into cooperative-computing-lab:master Jan 9, 2024
7 checks passed
@JinZhou5042 JinZhou5042 deleted the mem_leak branch January 14, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For modifications that fix a flaw in the code. TaskVine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants