You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by zellerh <gi...@git.apache.org> on 2017/06/07 15:48:29 UTC
[GitHub] incubator-trafodion pull request #1113: [TRAFODION-2636] Memory leak in meta...
GitHub user zellerh opened a pull request:
https://github.com/apache/incubator-trafodion/pull/1113
[TRAFODION-2636] Memory leak in metadata context
Fixing a memory leak found by Selva. See [[TRAFODION-2636]](https://issues.apache.org/jira/browse/TRAFODION-2636) for details. Also adding a method that helped me in debugging this issue.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zellerh/incubator-trafodion bug/cses_jan-17
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafodion/pull/1113.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1113
----
commit 9dd45d251fda6a103c5d90b6706babbea55f32fa
Author: Hans Zeller <hz...@apache.org>
Date: 2017-06-07T15:43:23Z
[TRAFODION-2636] Memory leak in metadata context
Fixing a memory leak found by Selva.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1113: [TRAFODION-2636] Memory leak in meta...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1113#discussion_r120693437
--- Diff: core/sql/optimizer/ControlDB.cpp ---
@@ -108,7 +108,12 @@ ControlDB::~ControlDB()
void ControlDB::setRequiredShape(ControlQueryShape *shape)
{
- delete requiredShape_;
+ if (requiredShape_)
+ {
+ delete requiredShape_->getShape();
+ delete requiredShape_;
--- End diff --
@selvaganesang can you explain this more? When I look at the definition of NADELETE, it explicitly says that NADELETE is **not** meant for NABasicObjects, and we are dealing with NABasicObjects here. Is it that you don't trust the method in which we put the heap pointer into NABasicObjects?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1113: [TRAFODION-2636] Memory leak in meta...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1113#discussion_r120684023
--- Diff: core/sql/common/NAMemory.h ---
@@ -468,6 +468,11 @@ NA_EIDPROC
NAMemoryType getType() { return type_; }
NA_EIDPROC
NABoolean getUsage(size_t* lastSegSize, size_t* freeSize, size_t* totalSize);
+ // for debugging
+NA_EIDPROC
+ NABoolean containsAddress(void *addr)
+ { return NABlock::blockHolding(firstBlk_, addr) != NULL; }
+
--- End diff --
Is this some kind of debugging function?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1113: [TRAFODION-2636] Memory leak in meta...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-trafodion/pull/1113
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1113: [TRAFODION-2636] Memory leak in meta...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1113#discussion_r120692519
--- Diff: core/sql/common/NAMemory.h ---
@@ -468,6 +468,11 @@ NA_EIDPROC
NAMemoryType getType() { return type_; }
NA_EIDPROC
NABoolean getUsage(size_t* lastSegSize, size_t* freeSize, size_t* totalSize);
+ // for debugging
+NA_EIDPROC
+ NABoolean containsAddress(void *addr)
+ { return NABlock::blockHolding(firstBlk_, addr) != NULL; }
+
--- End diff --
Yes, I added the following code to the GroupAttributes constructor to find this leak, out of thousands of allocations it did:
> const char *loc;
>
> if ((long) this < 0x8000000)
> loc = "c++h";
> else if ((long) this > 0x7ff000000000)
> loc = "stck";
> else if (CmpCommon::statementHeap() == NULL)
> loc = "nost";
> else if (CmpCommon::statementHeap()->containsAddress(this))
> loc = "stmt";
> else if (CmpCommon::contextHeap()->containsAddress(this))
> loc = "ctxt";
> else
> loc = "unkn";
>
> printf("GroupAttributes1, %#018lx %s\n", (long) this, loc);
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1113: [TRAFODION-2636] Memory leak in meta...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1113#discussion_r120685625
--- Diff: core/sql/common/NAMemory.h ---
@@ -468,6 +468,11 @@ NA_EIDPROC
NAMemoryType getType() { return type_; }
NA_EIDPROC
NABoolean getUsage(size_t* lastSegSize, size_t* freeSize, size_t* totalSize);
+ // for debugging
+NA_EIDPROC
+ NABoolean containsAddress(void *addr)
+ { return NABlock::blockHolding(firstBlk_, addr) != NULL; }
+
--- End diff --
Please ignore this comment. I should have read the commit message
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1113: [TRAFODION-2636] Memory leak in meta...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1113#discussion_r120683604
--- Diff: core/sql/optimizer/ControlDB.cpp ---
@@ -108,7 +108,12 @@ ControlDB::~ControlDB()
void ControlDB::setRequiredShape(ControlQueryShape *shape)
{
- delete requiredShape_;
+ if (requiredShape_)
+ {
+ delete requiredShape_->getShape();
+ delete requiredShape_;
--- End diff --
Though delete would also work fine because of the macro, I would prefer NADELETE here with the correct heap.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1113: [TRAFODION-2636] Memory leak in meta...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1113#discussion_r120712778
--- Diff: core/sql/optimizer/ControlDB.cpp ---
@@ -108,7 +108,12 @@ ControlDB::~ControlDB()
void ControlDB::setRequiredShape(ControlQueryShape *shape)
{
- delete requiredShape_;
+ if (requiredShape_)
+ {
+ delete requiredShape_->getShape();
+ delete requiredShape_;
--- End diff --
Yes, one thing I don't like about NABasicObject is that the heap pointer is junk for NABasicObjects that are allocated on the stack. Fortunately that's not a problem here. The allocation/deallocation of objects is pretty clear in this file. Thanks again for finding this leak!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1113: [TRAFODION-2636] Memory leak in meta...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1113#discussion_r120700381
--- Diff: core/sql/optimizer/ControlDB.cpp ---
@@ -108,7 +108,12 @@ ControlDB::~ControlDB()
void ControlDB::setRequiredShape(ControlQueryShape *shape)
{
- delete requiredShape_;
+ if (requiredShape_)
+ {
+ delete requiredShape_->getShape();
+ delete requiredShape_;
--- End diff --
If the NADELETE macro can't be used, then it is fine to stick with delete. However, NABasicObject also has the default constructor and hence it is possible to create objects without heap. I guess overloaded delete operator would take care of this too.
I preferred NADELETE macro because it is easy to read that it is being deleted from the heap.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---