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

Pterosaur · 2024-05-24T15:38:18Z

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 #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>

prgeor · 2024-05-24T15:47:18Z

tests/test_redis_ut.py

+    for _ in range(OP_COUNT):
+        t.set("big_data", fvs)
+        t.get("big_data")
+    assert psutil.Process(os.getpid()).memory_info().rss - rss < OP_COUNT


@Pterosaur os.getpid()).memory_info().rss - rss shouldn't this be close to ZERO?

It should be close to ZERO, but if there is a memory leak on the set/get OPs, we will got the following log:

$ python3 -m pytest --pdb -s -v tests/test_redis_ut.py::test_TableOpsMemoryLeak ============================================================= test session starts ============================================================== platform linux -- Python 3.10.12, pytest-6.2.5, py-1.10.0, pluggy-0.13.0 -- /usr/bin/python3 cachedir: .pytest_cache rootdir: /home/zegan/github/sonic/sonic-swss-common plugins: leaks-0.3.1 collected 1 item tests/test_redis_ut.py::test_TableOpsMemoryLeak FAILED >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> def test_TableOpsMemoryLeak(): OP_COUNT = 50000 app_db = swsscommon.DBConnector("APPL_DB", 0, True) t = swsscommon.Table(app_db, "TABLE") long_data = "x" * 100 fvs = swsscommon.FieldValuePairs([(long_data, long_data)]) rss = psutil.Process(os.getpid()).memory_info().rss for _ in range(OP_COUNT): t.set("long_data", fvs) t.get("long_data") > assert psutil.Process(os.getpid()).memory_info().rss - rss < OP_COUNT E AssertionError: assert (82194432 - 43737088) < 50000 E + where 82194432 = pmem(rss=82194432, vms=104538112, shared=21266432, text=2818048, lib=0, data=64155648, dirty=0).rss E + where pmem(rss=82194432, vms=104538112, shared=21266432, text=2818048, lib=0, data=64155648, dirty=0) = <bound method Process.memory_info of psutil.Process(pid=282163, name='python3', status='running', started='15:26:59')>() E + where <bound method Process.memory_info of psutil.Process(pid=282163, name='python3', status='running', started='15:26:59')> = psutil.Process(pid=282163, name='python3', status='running', started='15:26:59').memory_info E + where psutil.Process(pid=282163, name='python3', status='running', started='15:26:59') = <class 'psutil.Process'>(282163) E + where <class 'psutil.Process'> = psutil.Process E + and 282163 = <built-in function getpid>() E + where <built-in function getpid> = os.getpid tests/test_redis_ut.py:880: AssertionError >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> entering PDB >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PDB post_mortem >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > /home/zegan/github/sonic/sonic-swss-common/tests/test_redis_ut.py(880)test_TableOpsMemoryLeak() -> assert psutil.Process(os.getpid()).memory_info().rss - rss < OP_COUNT (Pdb) q

@Pterosaur if so, please delete the "long_data" key from the table and

assert psutil.Process(os.getpid()).memory_info().rss - rss == 0

good idea, I will create another PR for teardown.

prgeor · 2024-05-24T15:48:46Z

tests/test_redis_ut.py

+    rss = psutil.Process(os.getpid()).memory_info().rss
+    for _ in range(OP_COUNT):
+        t.set("big_data", fvs)
+        t.get("big_data")


@Pterosaur delete the "big_data" also after use?

Why delete it? I didn't test the del OPs, because the del OPs will not come to my SWIG code

qiluo-msft · 2024-05-24T16:05:26Z

pyext/swsscommon.i

@@ -156,7 +156,7 @@
    SWIG_Python_AppendOutput($result, temp);
 }

-%typemap(in, fragment="SWIG_AsPtr_std_string")
+%typemap(in, fragment="SWIG_AsVal_std_string")
    const std::vector<std::pair< std::string,std::string >,std::allocator< std::pair< std::string,std::string > > > &
        (std::vector< std::pair< std::string,std::string >,std::allocator< std::pair< std::string,std::string > > > temp,


temp is not a reference. Is it heavy copy? #Closed

It's not a function argument, just to tell the SWIG to generate a temp variable. The generated code looks like:

So, reference isn't available here.

qiluo-msft · 2024-05-24T22:59:30Z

pyext/swsscommon.i

@@ -205,8 +211,10 @@
            && !PyUnicode_Check(value)
            && !PyString_Check(value)) {
            $1 = 0;
+            Py_DECREF(item);


Py_DECREF

It's easy to forget one Py_DECREF. could you encapsulate and improve systematically? #Closed

Done, pleas check it.

Signed-off-by: Ze Gan <zegan@microsoft.com>

qiluo-msft · 2024-05-25T21:05:19Z

pyext/swsscommon.i

+            PySequence_GetItem($input, i), 
+            [](PyObject *ptr){
+                Py_DECREF(ptr);
+            });


This long statement is duplicated. Could you further refactor? #WontFix

Options I see:

use a new class to wrap the pointer

make_unique

1 is better.

It looks like not a good idea to use a new class/function in SWIG code.
Because I need to use some internal functions/MACRO that are generated in the wrap.cpp. So I cannot access them in the outside of this file.

For example, I tried to create a new file like swig_utils.h but got following error:

pyext/swig_utils.h:26:16: error: ‘SWIG_AsVal_std_string’ was not declared in this scope; did you mean ‘SWIGTYPE_p_std__string’? 26 | } else if (SWIG_AsVal_std_string(obj, &buffer) != SWIG_ERROR) { | ^~~~~~~~~~~~~~~~~~~~~ | SWIGTYPE_p_std__string

Because this function is generated in the swsscommon_wrap.c at compiling time, So I cannot access it in the outside.

This function signature isn't fixed so I cannot use a forward-declaration yet.

qiluo-msft · 2024-05-25T21:13:14Z

pyext/swsscommon.i

-        } else if (SWIG_AsPtr_std_string(value, &ptr)) {
-            temp.back().second = *ptr;
+        } else if (SWIG_AsVal_std_string(value, &str) != SWIG_ERROR) {
+            temp.back().second = str;
        } else {
            SWIG_fail;
        }


the if-elesif-else block is duplicated. Could you further refactor? #WontFix

I feel to extract a new function isn't a good idea in our case, same reason as above.

Pterosaur · 2024-05-26T01:06:38Z

/azpw run

mssonicbld · 2024-05-26T01:06:41Z

/AzurePipelines run

azure-pipelines · 2024-05-26T01:06:52Z

Azure Pipelines successfully started running 1 pipeline(s).

qiluo-msft · 2024-05-28T16:22:13Z

@prgeor Do you have more concern?

lguohan · 2024-05-28T16:38:13Z

@Pterosaur , thanks for fixing this, can you submit pr to get it fixed in 202311?

@praveenraja1

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>

@praveenraja1

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>

@praveenraja1

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>

prgeor · 2024-05-30T14:11:56Z

@prgeor Do you have more concern?

@Pterosaur @qiluo-msft so its OK to leak < 50000 bytes but not more than that

prgeor · 2024-05-30T14:12:36Z

I am referring to test_TableOpsMemoryLeak() this test

Pterosaur · 2024-05-30T14:34:32Z

@prgeor Do you have more concern?

@Pterosaur @qiluo-msft so its OK to leak < 50000 bytes but not more than that

@prgeor
This test will call set/get 50000 times in the loop, so as long as it leaks more than ONE byte in an iteration, it will be detected.
This case is just for testing the leakage of set/get, if there is another leakage point not in this loop, I don't expect that can be detected by this test case.

@praveenraja1

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 #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>

@praveenraja1

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>

@praveenraja1

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>

Fix memory leak

5dfe511

Signed-off-by: Ze Gan <zegan@microsoft.com>

Pterosaur requested a review from qiluo-msft May 24, 2024 15:38

Pterosaur mentioned this pull request May 24, 2024

Memory leaks in pmon daemons related to Redis set activity sonic-net/sonic-buildimage#17025

Closed

Pterosaur added the Request for 202311 Branch label May 24, 2024

mssonicbld added the Cherry Pick Conflict_202311 label May 24, 2024

prgeor reviewed May 24, 2024

View reviewed changes

qiluo-msft reviewed May 24, 2024

View reviewed changes

Refactor interface and reduce test memory consumption

682436b

Signed-off-by: Ze Gan <zegan@microsoft.com>

Pterosaur requested a review from qiluo-msft May 25, 2024 01:11

qiluo-msft reviewed May 25, 2024

View reviewed changes

Pterosaur requested review from qiluo-msft and prgeor May 26, 2024 01:06

Merge branch 'master' into memory_leak

d234536

qiluo-msft approved these changes May 28, 2024

View reviewed changes

lguohan merged commit 138b32e into sonic-net:master May 28, 2024
17 checks passed

Pterosaur added Included in 202311 Branch and removed Request for 202311 Branch Cherry Pick Conflict_202311 labels Aug 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Pterosaur commented May 24, 2024

prgeor May 24, 2024

Pterosaur May 24, 2024

prgeor May 25, 2024

Pterosaur May 30, 2024

prgeor May 24, 2024

Pterosaur May 24, 2024

qiluo-msft May 24, 2024 •

edited

Loading

Pterosaur May 24, 2024

qiluo-msft May 24, 2024 •

edited

Loading

Pterosaur May 24, 2024

qiluo-msft May 25, 2024 •

edited

Loading

qiluo-msft May 25, 2024

Pterosaur May 26, 2024 •

edited

Loading

qiluo-msft May 25, 2024 •

edited

Loading

Pterosaur May 26, 2024

Pterosaur commented May 26, 2024

mssonicbld commented May 26, 2024

azure-pipelines bot commented May 26, 2024

qiluo-msft commented May 28, 2024

lguohan commented May 28, 2024

prgeor commented May 30, 2024

prgeor commented May 30, 2024

Pterosaur commented May 30, 2024

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

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

Conversation

Pterosaur commented May 24, 2024

Description

Reason

Fix:

Add unit test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pterosaur May 26, 2024 • edited Loading

Choose a reason for hiding this comment

qiluo-msft May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pterosaur commented May 26, 2024

mssonicbld commented May 26, 2024

azure-pipelines bot commented May 26, 2024

qiluo-msft commented May 28, 2024

lguohan commented May 28, 2024

prgeor commented May 30, 2024

prgeor commented May 30, 2024

Pterosaur commented May 30, 2024

qiluo-msft May 24, 2024 •

edited

Loading

qiluo-msft May 24, 2024 •

edited

Loading

qiluo-msft May 25, 2024 •

edited

Loading

Pterosaur May 26, 2024 •

edited

Loading

qiluo-msft May 25, 2024 •

edited

Loading