From 5dfe511b2fa2b77bd70a246cef39b83485b002fb Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Fri, 24 May 2024 15:19:22 +0000 Subject: [PATCH 1/2] Fix memory leak Signed-off-by: Ze Gan --- .azure-pipelines/build-template.yml | 2 +- pyext/swsscommon.i | 20 ++++++++++++++------ tests/test_redis_ut.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/.azure-pipelines/build-template.yml b/.azure-pipelines/build-template.yml index f2a0ee496..d1813c23a 100644 --- a/.azure-pipelines/build-template.yml +++ b/.azure-pipelines/build-template.yml @@ -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 diff --git a/pyext/swsscommon.i b/pyext/swsscommon.i index 81b9cdb19..bf4f7b3e3 100644 --- a/pyext/swsscommon.i +++ b/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::allocator< std::pair< std::string,std::string > > > & (std::vector< std::pair< std::string,std::string >,std::allocator< std::pair< std::string,std::string > > > temp, int res) { @@ -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; } + Py_DECREF(item); } $1 = &temp; } @@ -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); @@ -205,8 +211,10 @@ && !PyUnicode_Check(value) && !PyString_Check(value)) { $1 = 0; + Py_DECREF(item); break; } + Py_DECREF(item); } } diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py index b88a3539c..5ed9678a8 100644 --- a/tests/test_redis_ut.py +++ b/tests/test_redis_ut.py @@ -1,5 +1,6 @@ import os import time +import psutil import pytest import multiprocessing from threading import Thread @@ -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") + assert psutil.Process(os.getpid()).memory_info().rss - rss < OP_COUNT + From 682436b8d03beec2928f7e32528c39e990632736 Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Fri, 24 May 2024 23:45:35 +0000 Subject: [PATCH 2/2] Refactor interface and reduce test memory consumption Signed-off-by: Ze Gan --- pyext/swsscommon.i | 33 ++++++++++++++++++--------------- tests/test_redis_ut.py | 8 ++++---- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/pyext/swsscommon.i b/pyext/swsscommon.i index bf4f7b3e3..2bf953b11 100644 --- a/pyext/swsscommon.i +++ b/pyext/swsscommon.i @@ -56,6 +56,8 @@ #include "zmqclient.h" #include "zmqconsumerstatetable.h" #include "zmqproducerstatetable.h" +#include +#include %} %include @@ -163,13 +165,16 @@ res = SWIG_OK; for (int i = 0; i < PySequence_Length($input); ++i) { 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); + std::unique_ptr > item( + PySequence_GetItem($input, i), + [](PyObject *ptr){ + Py_DECREF(ptr); + }); + if (!PyTuple_Check(item.get()) || PyTuple_Size(item.get()) != 2) { SWIG_fail; } - PyObject *key = PyTuple_GetItem(item, 0); - PyObject *value = PyTuple_GetItem(item, 1); + PyObject *key = PyTuple_GetItem(item.get(), 0); + PyObject *value = PyTuple_GetItem(item.get(), 1); std::string str; if (PyBytes_Check(key)) { @@ -177,7 +182,6 @@ } else if (SWIG_AsVal_std_string(key, &str) != SWIG_ERROR) { temp.back().first = str; } else { - Py_DECREF(item); SWIG_fail; } if (PyBytes_Check(value)) { @@ -185,10 +189,8 @@ } else if (SWIG_AsVal_std_string(value, &str) != SWIG_ERROR) { temp.back().second = str; } else { - Py_DECREF(item); SWIG_fail; } - Py_DECREF(item); } $1 = &temp; } @@ -196,14 +198,17 @@ %typemap(typecheck) const std::vector< std::pair< std::string,std::string >,std::allocator< std::pair< std::string,std::string > > > &{ $1 = 1; for (int i = 0; i < PySequence_Length($input); ++i) { - PyObject *item = PySequence_GetItem($input, i); - if (!PyTuple_Check(item) || PyTuple_Size(item) != 2) { + std::unique_ptr > item( + PySequence_GetItem($input, i), + [](PyObject *ptr){ + Py_DECREF(ptr); + }); + if (!PyTuple_Check(item.get()) || PyTuple_Size(item.get()) != 2) { $1 = 0; - Py_DECREF(item); break; } - PyObject *key = PyTuple_GetItem(item, 0); - PyObject *value = PyTuple_GetItem(item, 1); + PyObject *key = PyTuple_GetItem(item.get(), 0); + PyObject *value = PyTuple_GetItem(item.get(), 1); if (!PyBytes_Check(key) && !PyUnicode_Check(key) && !PyString_Check(key) @@ -211,10 +216,8 @@ && !PyUnicode_Check(value) && !PyString_Check(value)) { $1 = 0; - Py_DECREF(item); break; } - Py_DECREF(item); } } diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py index 5ed9678a8..bf395a34f 100644 --- a/tests/test_redis_ut.py +++ b/tests/test_redis_ut.py @@ -871,11 +871,11 @@ 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)]) + 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("big_data", fvs) - t.get("big_data") + t.set("long_data", fvs) + t.get("long_data") assert psutil.Process(os.getpid()).memory_info().rss - rss < OP_COUNT