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

[swig]: Fix swig template memory leak on issue 17025 #878

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

Pterosaur
Copy link
Contributor

@Pterosaur Pterosaur commented May 29, 2024

Fix the issue sonic-net/sonic-buildimage#17025 about Redis set activity

Wait PR: #836 cherry-pick to 202311

Description

The issue reports a memory leak on the Redis set operations

Reason

  1. Didn't decrease the reference count after PySequence_GetItem
  2. Use the inappropriate Swig API and didn't release the string of SWIG_AsPtr_std_string

Fix:

  1. Refer PR: Fix swig template memory leak #859 from @praveenraja1
  2. Replace the API SWIG_AsPtr_std_string to SWIG_AsVal_std_string

Add unit test

To monitor there is no dramatic memory increment after a huge amount of Redis set operations.

@Pterosaur Pterosaur changed the title [swig]: Fix swig template memory leak on issue 17025 (#876) [swig]: Fix swig template memory leak on issue 17025 May 29, 2024
@Pterosaur Pterosaur force-pushed the 202311_memory_leak branch from 99884f8 to a66dcbd Compare May 29, 2024 02:46
Fix the issue sonic-net/sonic-buildimage#17025 about Redis set activity

Description
The issue reports a memory leak on the Redis set operations

Reason
Didn't decrease the reference count after PySequence_GetItem
Use the inappropriate Swig API and didn't release the string of SWIG_AsPtr_std_string
Fix:
Refer PR: Fix swig template memory leak sonic-net#859 from @praveenraja1
Replace the API SWIG_AsPtr_std_string to SWIG_AsVal_std_string
Add unit test
To monitor there is no dramatic memory increment after a huge amount of Redis set operations.

Signed-off-by: Ze Gan <zegan@microsoft.com>
@Pterosaur Pterosaur force-pushed the 202311_memory_leak branch from a66dcbd to 7424607 Compare May 29, 2024 03:05
@lguohan
Copy link
Contributor

lguohan commented May 29, 2024

@Pterosaur , please check why build failure.

@Pterosaur Pterosaur marked this pull request as draft May 29, 2024 06:04
@Pterosaur
Copy link
Contributor Author

@Pterosaur , please check why build failure.

@lguohan We should cherry-pick #836 to 202311 firstly.

@Pterosaur Pterosaur marked this pull request as ready for review May 29, 2024 09:46
@Pterosaur
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Pterosaur
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Pterosaur Pterosaur requested review from lguohan and qiluo-msft May 31, 2024 09:24
@Pterosaur
Copy link
Contributor Author

@lguohan @qiluo-msft Please help to approve and merge this to 202311

@keboliu keboliu requested a review from yxieca June 3, 2024 02:51
@keboliu
Copy link
Collaborator

keboliu commented Jun 3, 2024

@yxieca would you please help to merge?

@yxieca yxieca merged commit dea9561 into sonic-net:202311 Jun 3, 2024
14 checks passed
@Pterosaur Pterosaur deleted the 202311_memory_leak branch June 4, 2024 04:15
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.

5 participants