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.

Sign up for GitHub

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

Jump to bottom

Fix memory leak. #828

Merged
merged 2 commits into from
Sep 30, 2021
Merged

Fix memory leak. #828

merged 2 commits into from
Sep 30, 2021

Conversation

llapx
Copy link
Contributor

@llapx llapx commented Sep 28, 2021

Signed-off-by: Lei Liu [email protected]

@llapx
Fix memory leak.
75b9de6 
Signed-off-by: Lei Liu <[email protected]>
aprotyas
aprotyas approved these changes Sep 28, 2021
View reviewed changes
Copy link
Member

@aprotyas aprotyas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM under #822's context.

Although, I wonder if there was a case to capture this concern through a unit test as well? I'm not sure, but this seems like something you would want to test for (i.e. is the client/server correctly "taking" the response/request).

@llapx
Copy link
Contributor Author

llapx commented Sep 28, 2021 •
edited
Loading

In my option, it's a memory leak bug which may affect many test cases. It's nessary to write test cases for memory leak, but it's may be a big topic.

@aprotyas
Copy link
Member

For reviewers, just reproducing the memory usage charts from #822 (comment)
test_client_before
test_client_after

The same observations should hold for services using rclpy.
As such, I think this closes #822.

iuhilnehc-ynos
iuhilnehc-ynos reviewed Sep 28, 2021
View reviewed changes
Copy link
Contributor

@iuhilnehc-ynos iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

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

nitpick.

rclpy/src/rclpy/client.cpp Outdated
@@ -138,7 +138,6 @@ Client::take_response(py::object pyresponse_type)

result_tuple[1] = convert_to_py(taken_response.get(), pyresponse_type);
// result_tuple now owns the message
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no reason to keep this comment, I suggest deleting it.

@fujitatomoya fujitatomoya mentioned this pull request Sep 29, 2021
Closed
fujitatomoya
fujitatomoya approved these changes Sep 29, 2021
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm with @iuhilnehc-ynos 's comment.

@fujitatomoya
Copy link
Collaborator

@aprotyas it seems that memory usage doe not change on the graph chart between before and after? it still increases the memory usage constantly on client side? just checking.

@fujitatomoya
Copy link
Collaborator

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@aprotyas
Copy link
Member

@aprotyas it seems that memory usage doe not change on the graph chart between before and after? it still increases the memory usage constantly on client side? just checking.

No no, the first image is from before, and the second image is from after this PR.
In the second image, you see that the memory usage does not continuously increase (stays consistent around 60 MB). So I believe the PR fixes the memory leak.

@fujitatomoya
Copy link
Collaborator

@aprotyas Ah, i see it. thanks!

@llapx
Remove comment.
6b94c0f 
Signed-off-by: Lei Liu <[email protected]>
iuhilnehc-ynos
iuhilnehc-ynos approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@iuhilnehc-ynos iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

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

LGTM.

Re-CI (error happened on Window because of rti package installation, rebuild with @fujitatomoya setting on ci_launcher):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@fujitatomoya fujitatomoya merged commit ac13665 into ros2:master Sep 30, 2021
@aprotyas
Copy link
Member

Should this be backported?

@fujitatomoya
Copy link
Collaborator

@Mergifyio backport galactic

mergify bot pushed a commit that referenced this pull request Oct 25, 2021
@llapx
Fix memory leak. (#828)
58e7aea 
* Fix memory leak.

Signed-off-by: Lei Liu <[email protected]>

Co-authored-by: Chen Lihui <[email protected]>
Co-authored-by: Abrar Rahman Protyasha <[email protected]>
(cherry picked from commit ac13665)
@mergify mergify bot mentioned this pull request Oct 25, 2021
Merged
@mergify
Copy link
Contributor

mergify bot commented Oct 25, 2021

backport galactic

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aprotyas aprotyas aprotyas approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@iuhilnehc-ynos iuhilnehc-ynos iuhilnehc-ynos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@llapx @aprotyas @fujitatomoya @iuhilnehc-ynos