Fix not deleted\initialized memory #661
Conversation
|
👋 Hi rafiw! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
src/core/nixl_agent.cpp
Outdated
| @@ -91,6 +91,8 @@ nixlAgentData::nixlAgentData(const std::string &name, | |||
| useEtcd = false; | |||
| NIXL_DEBUG << "NIXL ETCD is disabled"; | |||
| } | |||
| #else | |||
There was a problem hiding this comment.
can we do
#if HAVE_ETCD
if (getenv("NIXL_ETCD_ENDPOINTS")) {
useEtcd = true;
NIXL_DEBUG << "NIXL ETCD is enabled";
} else
#endif
{
useEtcd = false;
NIXL_DEBUG << "NIXL ETCD is disabled";
}
| @@ -404,6 +405,8 @@ nixlDocaEngine::nixlDocaInitNotif (const std::string &remote_agent, | |||
| delete notif->recv_mmap; | |||
| delete notif->recv_barr; | |||
|
|
|||
| delete notif; | |||
There was a problem hiding this comment.
seems aldo need to free notif->send_addr
rafiw
force-pushed
the
valgrind_issues
branch
2 times, most recently
from
5762f9f to
0e56dd8
Compare
| @@ -159,7 +159,6 @@ nixl_status_t | |||
| nixlObjEngine::deregisterMem(nixlBackendMD *meta) { | |||
| nixlObjMetadata *obj_md = static_cast<nixlObjMetadata *>(meta); | |||
| if (obj_md) { | |||
| std::unique_ptr<nixlObjMetadata> obj_md_ptr = std::unique_ptr<nixlObjMetadata>(obj_md); | |||
There was a problem hiding this comment.
Why are you removing this line? Who's deleting the metadata instance then?
There was a problem hiding this comment.
your right delete is needed
There was a problem hiding this comment.
Again, is there a problem with this code? Why are you substituting RAII-style memory management with inferior manual object deletion?
rafiw
requested review from
aranadive,
ovidiusm,
yosefe and
gleon99
as code owners
| @@ -89,6 +90,7 @@ RUN echo "INFO: Starting custom UCX build..." && \ | |||
| ./autogen.sh && \ | |||
| echo "INFO: Building UCX..." && \ | |||
| ./contrib/configure-release --with-cuda=/usr/local/cuda \ | |||
| --enable-debug \ | |||
There was a problem hiding this comment.
I see why you need it but this is not the right way to do it. We want nixlbench container builds to happen in release mode by default, so that we can do performance testing. If we need debug mode, it should be a parameter.
I would rather add the changes in this file to #614 Would that be OK with you?
Signed-off-by: Rafi Wiener <[email protected]>
rafiw
force-pushed
the
valgrind_issues
branch
2 times, most recently
from
8b222b3 to
90d9b33
Compare
Signed-off-by: Rafi Wiener <[email protected]>
What?
fix memory issues
Why?
since it's needed