fix memory leak in regions(cpu backend) & rgb2ycbcr(api level) #1666

9prady9 · 2016-12-07T12:22:03Z

Fixes Memory Leak in CPU regions #1664 . Verified that the leak is not being reported in Valgrind anymore.
Fixes Memory leak in rgb2ycbcr #1672 . Verified that the leak is not being reported in Valgrind anymore.

umar456 · 2016-12-08T16:27:15Z

src/backend/cpu/kernel/regions.hpp

@@ -178,6 +178,9 @@ void regions(Array<T> out, const Array<char> in, af_connectivity connectivity)
        }
    }

+    for (auto& mapElem: lmap)
+        delete mapElem.second;


It would be better if you stored unique_ptrs in the lmap so you don't have to do this.

I thought about unique pointers, but then i thought why add another abstraction instead of just cleaning them off at the end since this object's definition and usage is local to that function. Will using unique pointers make it more efficient ?

unique ptrs aren't abstractions. They are there to make this type of thing from not happening again. You should almost never use raw pointers

umar456 · 2016-12-13T01:12:26Z

src/backend/cpu/kernel/regions.hpp

@@ -103,7 +104,8 @@ void regions(Array<T> out, const Array<char> in, af_connectivity connectivity)
    T    *out_ptr = out.get();

    // Map labels
-    typedef typename std::map<T, LabelNode<T>* > label_map_t;
+    typedef typename std::unique_ptr< LabelNode<T> > UnqLabelPtr;


Use consistent naming conventions within a file.

umar456 · 2016-12-13T01:13:47Z

src/backend/cpu/kernel/regions.hpp

                    // Insert new label in map
-                    LabelNode<T> *node = new LabelNode<T>(label);
-                    lmap.insert(std::pair<T, LabelNode<T>* >(label, node));
+                    lmap.insert(std::pair<T, UnqLabelPtr>(label, UnqLabelPtr(new LabelNode<T>(label))));


use make_pair instead of the pair constructor.

9prady9 · 2016-12-14T03:58:02Z

build arrayfire windows cpu ci

Please look at the updated changes.

shehzan10 · 2016-12-15T16:09:07Z

src/api/c/ycbcr_rgb.cpp

+    indices[2] = {1, 1, 1};
+    Array<T> Y = createSubArray(input, indices, false);
+
+    indices[2] = {2, 2, 1};


If you are making another commit in this PR, can you add a comment saying why you are only assigning to indices[2] in all the 3 cases. I think it would help code readability.

@shehzan10 Because he is reading the three channels into 3 different arrays ?

@9prady9 Can you check if something like createSubArray(input, {af_span, af_span, {1,1,1}}, false); works.

I have left comments before the beginning of extraction of channels. In any case, i don't have any other commits left in this PR.

I know why he is doing it, but it took me a bit to realize it. That's why I said "if you are making another commit", because its not that much of a priority. It just helps anyone in the future who is going to read it.

@pavanky I checked by running the existing tests, why would we need copy's of original Array ? Just indexed Array's are sufficient right ?

Is there a possibility that the meta-data may not be properly altered ?

@9prady9 Check my comment again. I did not say anything about copy, I was wondering if array initializers work instead of using vectors.

9prady9 · 2016-12-16T00:09:53Z

That is the reason I asked for clarification. I didn't get what you were concerned about. They seem to be working.

…

On Thu, Dec 15, 2016, 10:06 PM Pavan Yalamanchili ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/api/c/ycbcr_rgb.cpp <#1666>: > // get Array objects for corresponding channel views - Array<T> X = getArray<T>(ch1Temp); - Array<T> Y = getArray<T>(ch2Temp); - Array<T> Z = getArray<T>(ch3Temp); + const Array<T>& input = getArray<T>(in); + std::vector<af_seq> indices(4, af_span); + + indices[2] = {0, 0, 1}; + Array<T> X = createSubArray(input, indices, false); + + indices[2] = {1, 1, 1}; + Array<T> Y = createSubArray(input, indices, false); + + indices[2] = {2, 2, 1}; @9prady9 <https://github.com/9prady9> Check my comment again. I did not say anything about copy, I was wondering if array initializers work instead of using vectors. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1666>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADHnOqjvueNe8FikW7IyIzJrBozPoeOyks5rIWx0gaJpZM4LGiAw> .

umar456 · 2016-12-19T16:05:07Z

src/backend/cpu/kernel/regions.hpp

-    typedef typename std::map<T, LabelNode<T>* > label_map_t;
-    typedef typename label_map_t::iterator label_map_iterator_t;
+    typedef typename std::unique_ptr< LabelNode<T> > UnqLabelPtr;
+    typedef typename std::map<T, UnqLabelPtr > LabelMap;


This is only used once. It is an unnecessary indirection but not a big deal. Your call

Also. Use the using keyword for type aliasing. They are easier to read and more powerful.

using LabelMap = typename std::map<T, UnqLabelPtr>;

This is just one character smaller..

It's easier (for me) to read because it uses the assignment style to define the alias, not because it is shorter.

umar456 · 2016-12-19T16:10:34Z

src/backend/cpu/kernel/regions.hpp

+    for (int j = 0; j < (int)inDims[1]; j++) {
+        for (int i = 0; i < (int)inDims[0]; i++) {
+            int idx = j * inDims[0] + i;
+            if (inPtr[idx] != 0) {
                std::vector<T> l;


It would be better if this was a std::array instead of a std::vector. Since std::arrays are allocated on the stack, you don't get the penalty of calling malloc.

I know its not part of this PR. Perhaps in another issue.

pavanky

Looks good.

9prady9 added the fix label Dec 7, 2016

9prady9 added this to the v3.4.2 milestone Dec 7, 2016

9prady9 requested review from umar456 and shehzan10 December 8, 2016 05:35

shehzan10 approved these changes Dec 8, 2016

View reviewed changes

umar456 reviewed Dec 8, 2016

View reviewed changes

9prady9 force-pushed the cpu_regions_mem_leak branch from 130ea04 to fb645bb Compare December 9, 2016 04:33

9prady9 requested a review from umar456 December 9, 2016 04:33

shehzan10 added the ready to merge label Dec 13, 2016

umar456 previously requested changes Dec 14, 2016

View reviewed changes

fix memory leak in regions cpu backend

f8590d4

9prady9 force-pushed the cpu_regions_mem_leak branch from fb645bb to f8590d4 Compare December 14, 2016 03:55

pavanky mentioned this pull request Dec 14, 2016

Memory leak in rgb2ycbcr #1672

Closed

9prady9 requested a review from umar456 December 14, 2016 11:52

shehzan10 removed the ready to merge label Dec 14, 2016

9prady9 changed the title ~~fix memory leak in regions cpu backend~~ fix memory leak in regions(cpu backend) & rgb2ycbcr(api level) Dec 14, 2016

fix memory leak in rgb ycbcr conversion fns

d1e61c2

shehzan10 reviewed Dec 15, 2016

View reviewed changes

9prady9 requested a review from pavanky December 19, 2016 14:49

umar456 approved these changes Dec 19, 2016

View reviewed changes

shehzan10 added the ready to merge label Dec 19, 2016

pavanky approved these changes Dec 19, 2016

View reviewed changes

shehzan10 merged commit ac943db into arrayfire:hotfix-3.4.2 Dec 19, 2016

9prady9 deleted the cpu_regions_mem_leak branch December 23, 2016 06:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix memory leak in regions(cpu backend) & rgb2ycbcr(api level) #1666

fix memory leak in regions(cpu backend) & rgb2ycbcr(api level) #1666

9prady9 commented Dec 7, 2016 •

edited

umar456 Dec 8, 2016

9prady9 Dec 8, 2016

umar456 Dec 8, 2016

umar456 Dec 13, 2016

umar456 Dec 13, 2016

9prady9 commented Dec 14, 2016

shehzan10 Dec 15, 2016

pavanky Dec 15, 2016

9prady9 Dec 15, 2016

shehzan10 Dec 15, 2016

9prady9 Dec 15, 2016

9prady9 Dec 15, 2016

pavanky Dec 15, 2016

9prady9 commented Dec 16, 2016 via email

umar456 Dec 19, 2016

pavanky Dec 19, 2016

umar456 Dec 19, 2016

umar456 Dec 19, 2016

pavanky left a comment

fix memory leak in regions(cpu backend) & rgb2ycbcr(api level) #1666

fix memory leak in regions(cpu backend) & rgb2ycbcr(api level) #1666

Conversation

9prady9 commented Dec 7, 2016 • edited

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

9prady9 commented Dec 14, 2016

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

Choose a reason for hiding this comment

9prady9 commented Dec 16, 2016 via email

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

pavanky left a comment

Choose a reason for hiding this comment

9prady9 commented Dec 7, 2016 •

edited