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

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

Merged
merged 3 commits into from
May 28, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5dfe511
Fix memory leak
Pterosaur May 24, 2024
682436b
Refactor interface and reduce test memory consumption
Pterosaur May 24, 2024
d234536
Merge branch 'master' into memory_leak
Pterosaur May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .azure-pipelines/build-template.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ jobs:
displayName: "Install gcovr 5.2 (for --exclude-throw-branches support)"
- script: |
set -ex
sudo pip install Pympler==0.8
sudo pip install Pympler==0.8 pytest
sudo apt-get install -y redis-server
sudo sed -i 's/notify-keyspace-events ""/notify-keyspace-events AKE/' /etc/redis/redis.conf
sudo sed -ri 's/^# unixsocket/unixsocket/' /etc/redis/redis.conf
Expand Down
20 changes: 14 additions & 6 deletions pyext/swsscommon.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

@qiluo-msft qiluo-msft May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

image

So, reference isn't available here.

int res) {
Expand All @@ -165,25 +165,30 @@
temp.push_back(std::pair< std::string,std::string >());
PyObject *item = PySequence_GetItem($input, i);
if (!PyTuple_Check(item) || PyTuple_Size(item) != 2) {
Py_DECREF(item);
SWIG_fail;
}
PyObject *key = PyTuple_GetItem(item, 0);
PyObject *value = PyTuple_GetItem(item, 1);
std::string *ptr = (std::string *)0;
std::string str;

if (PyBytes_Check(key)) {
temp.back().first.assign(PyBytes_AsString(key), PyBytes_Size(key));
} else if (SWIG_AsPtr_std_string(key, &ptr)) {
temp.back().first = *ptr;
} else if (SWIG_AsVal_std_string(key, &str) != SWIG_ERROR) {
temp.back().first = str;
} else {
Py_DECREF(item);
SWIG_fail;
}
if (PyBytes_Check(value)) {
temp.back().second.assign(PyBytes_AsString(value), PyBytes_Size(value));
} 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 {
Py_DECREF(item);
SWIG_fail;
}
Copy link
Contributor

@qiluo-msft qiluo-msft May 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Py_DECREF(item);
}
$1 = &temp;
}
Expand All @@ -194,6 +199,7 @@
PyObject *item = PySequence_GetItem($input, i);
if (!PyTuple_Check(item) || PyTuple_Size(item) != 2) {
$1 = 0;
Py_DECREF(item);
break;
}
PyObject *key = PyTuple_GetItem(item, 0);
Expand All @@ -205,8 +211,10 @@
&& !PyUnicode_Check(value)
&& !PyString_Check(value)) {
$1 = 0;
Py_DECREF(item);
Copy link
Contributor

@qiluo-msft qiluo-msft May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Py_DECREF

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, pleas check it.

break;
}
Py_DECREF(item);
}
}

Expand Down
28 changes: 28 additions & 0 deletions tests/test_redis_ut.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import os
import time
import psutil
import pytest
import multiprocessing
from threading import Thread
Expand Down Expand Up @@ -851,3 +852,30 @@ def test_SmartSwitchDBConnector():
assert tbl.get("dputest2")[1][1] == ("dashfield2", "dashvalue2")
assert len(SonicDBConfig.getDbKeys()) == len(global_db_config_json["INCLUDES"])


def test_TableSetBinary():
app_db = swsscommon.DBConnector("APPL_DB", 0, True)
t = swsscommon.Table(app_db, "TABLE")
buff = b""
for i in range(0, 256):
buff += bytes([i])
buff = buff.decode('latin-1')
fvs = swsscommon.FieldValuePairs([("binary", buff)])
t.set("binary", fvs)
(status, fvs) = t.get("binary")
assert status == True
assert fvs[0][1] == buff


def test_TableOpsMemoryLeak():
OP_COUNT = 50000
app_db = swsscommon.DBConnector("APPL_DB", 0, True)
t = swsscommon.Table(app_db, "TABLE")
big_data = "x" * 10000
fvs = swsscommon.FieldValuePairs([(big_data, big_data)])
rss = psutil.Process(os.getpid()).memory_info().rss
for _ in range(OP_COUNT):
t.set("big_data", fvs)
t.get("big_data")
Copy link
Contributor

Choose a reason for hiding this comment

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

@Pterosaur delete the "big_data" also after use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

assert psutil.Process(os.getpid()).memory_info().rss - rss < OP_COUNT
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, I will create another PR for teardown.


Loading