0

I'm trying to speed up my (qt c++ opencv) program which should count number of colors in my photos for future filtering. There is no leak in single-threaded approach, but it is very slow.

With adding 8 threads I've already speed up this process up to 5x times.

The problem starts when I switch my program to multithreading.

There is a HUGE memory leak! http://snag.gy/cHRrS.jpg

Following this advises (https://stackoverflow.com/a/12859444) I prevented from subclassing QThread and implementing run().

Here is a for loop for counting each pixel in every new image shifted by 1 pixel:

ColorCounterController *cntrl[arrSize];

for (int i = 0; i < box; i++)//x
{
    for (int j = 0; j < box; ++j)//y
    {
        cv::Mat res=process(image,i,j);


        //Using 1 core
        //colors=ColorDetectController::getInstance()->colorsCount(res);

        //Using all 8 cores
        cntrl[cnt2%arrSize]= new ColorCounterController(res,this);

        ++cnt2;
    }

    ++cnt;
    emit setStatusProgressSignal((int)(cnt/amnt*100));
}

delete[] *cntrl;

Comments:

When using 1 core (above code) I have singleton to run colorsCount(res) function. In case of 8 cores I use almost the same function but called from ColorCounterController.

class ColorCounterController : public QObject{
Q_OBJECT
private:
QThread thread;
ColorCounter *colorCntr;
Pixalate *pixelate;
private slots:
void freecolorCntr(){
    delete colorCntr;
}
public:
ColorCounterController(const cv::Mat &image,Pixalate *pxobj) {
    colorCntr= new ColorCounter();
    colorCntr->setimageThread(image);
    colorCntr->moveToThread(&thread);
    connect(&thread, SIGNAL(started()), colorCntr, SLOT(colorsCountThread()));
    connect(colorCntr, SIGNAL(finished()), &thread, SLOT(quit()));
    connect(colorCntr, SIGNAL(finished()), colorCntr, SLOT(deleteLater()));
        connect(colorCntr, SIGNAL(results(int)), pxobj, SLOT(results(int)));
    thread.start();
}

    ~ColorCounterController() {
    thread.quit();
    thread.wait();
     qDebug() << QString("Controller quit wait");
    //delete colorCntr; //err
}

I suppose that leak is in ColorCounterController constructor:

        colorCntr= new ColorCounter();

But how to avoid it? This code causing an error. in a destructor:

//delete colorCntr; //err

and in a constructor:

//connect(&thread, SIGNAL(finished()), &thread, SLOT(deleteLater()));

Please help!

P.S.

I changed this

delete[] *cntrl;

to this

    for (int i = 0; i < arrSize; i++){
    if (cntrl[i])
        delete cntrl[i];
    }

and NULL for all pointers at the beginning before cntrl[cnt2%arrSize]

Nothing changed

P.P.S. In case you want to contribute to this question: https://github.com/ivanesses/curiosity

Community
  • 1
  • 1
  • What error is the delete colorCntr; causing? Is it compiler or runtime? What compilation flags are you using? Also... Can you provide more than a un-marked, untitled, graph to represent the memory leak? http://faculty.spokanefalls.edu/InetShare/AutoWebs/AsaB/GoodGraphs.pdf – Lilith Daemon Jul 23 '15 at 16:51
  • Run-time error Read access violation http://snag.gy/iyNku.jpg – avagrkjllkjfdser3 Jul 23 '15 at 17:34
  • About graph. Sorry for that mess. It just shows that I'm losing almost half of my memory (8/16Gb) in 1minute. – avagrkjllkjfdser3 Jul 23 '15 at 17:40
  • "What compilation flags are you using?" - I use compilator x64/vc12 from VS2013 with Qt default settings This is some .pro settings in Qt QT += core gui greaterThan(QT_MAJOR_VERSION, 4): QT += widgets TARGET = Curiosity TEMPLATE = app – avagrkjllkjfdser3 Jul 23 '15 at 17:44
  • Why not to use a memory profiler? Form the code it's quite hard to understand what can be the issue: raw pointers etc. I understand this is Qt style, but anyway... Try a profiler! – Artem Razin Jul 23 '15 at 19:55
  • How about using a Smart Pointer instead to it deletes the memory when it's no longer needed? Qt offers several smart pointers... – karlphillip Jul 23 '15 at 21:44
  • @ArtemRazin I need some profiler. But program is too small. I have no experience in profiling yet. – avagrkjllkjfdser3 Jul 23 '15 at 23:44
  • @karlphillip Trying to choose appropriate smart pointer... – avagrkjllkjfdser3 Jul 23 '15 at 23:44
  • Sorry, but code concept is awful. Advises you used for `QThread` is right, but you rebuild it isn't correct. Thread and class you want to parallel must be separate instance. At some places you defined threads and object, which contain methods for handling your task(like calculate or somethink like that). Then connect theads signals and slots with objects signal-slots. Just like in [post](http://stackoverflow.com/a/12859444) you are linked up. Moreover, i can't understand why you firstly connect `colorCntr` with `deleteLater()` slot, and after call delete? – t3ft3l--i Jul 24 '15 at 12:43
  • @t3ft3l--i Awful) yes. Haven't seen better approach to make it work in parallel. "Thread and class you want to parallel must be separate instance." -how is that? After called delete because nothing happened. – avagrkjllkjfdser3 Jul 24 '15 at 15:05

1 Answers1

1

2 problems cause leakage:

  1. pointers to new ColorCounterController objects will be lost forever (and their memory leaked) as the body of the "for" loop will run N times (N=box*box) creating N ColorCounterController objects but only pointers to 8 of them will fit into the array which you later use to delete objects.

  2. cntrl is an array of pointers. You need to iterate through it and call delete (plain delete, not delete[]) on each of it's elements.

  3. as found by the OP: must use imageThread.release(); instead of imageThread.deallocate()

V-R
  • 1,309
  • 16
  • 32
  • I see why http://stackoverflow.com/a/21731939/5148638 But leak is still there Nothing changed I added for (int i = 0; i < arrSize; i++){ if (cntrl[i]) delete cntrl[i]; } and NULL for all pointers at the beginning before cntrl[cnt2%arrSize] – avagrkjllkjfdser3 Jul 23 '15 at 17:27
  • Sorry but without a http://sscce.org this question does not seem to be answerable anymore. – V-R Jul 23 '15 at 18:07
  • If you cannot easily create a minimal version of the code exhibiting the problem, think about the expected / necessary lifetime of the allocated memory block, compare that to it's actual lifetime. It might look like a "leak" when you interrupt the program, while if it runs through it should execute the delete loop - does it? That last point can be checked either via debugger breakpoint or by printf-debugging to some log file. Good luck! – V-R Jul 23 '15 at 18:18
  • 1
    Also instead of checking for null before deleting a ptr - it is completely harmless to delete a null - set the ptr to null upon deletion! Just in case someone might mistakenly try to delete it again (would be a runtime error) or read/write memory via the ptr (even worse runtime errors). Dereferencing a null ptr would also be a runtime error but at least the behavior will be deterministic and easier to debug. You get non-deterministic behavior because the raw memory the process gets to work with is usually uninitialized and contains random garbage, potentially different with each execution. – V-R Jul 23 '15 at 18:34
  • hmm not sure i can build your project on my mac, but by looking at pixalate.cpp it seems that pointers to new ColorCounterController objects will be lost forever (and their memory leaked) as the "for" loop will run N times (N=box*box) creating N ColorCounterController objects but only pointers to 8 of them will fit into the array which you later loop over and delete. – V-R Jul 23 '15 at 20:17
  • On second thought, the code snippets in the question were enough to figure out the answer (and put the problems in spotlight), link to project didn't change anything. If the answer is complete now, and if you like it, please mark it as confirmed / accepted. – V-R Jul 23 '15 at 20:43
  • Really good bugs! But still it does not clean after itself https://github.com/ivanesses/curiosity/commit/55442322271d88b2296a550c748b96daa0909452 Increased array and everything should be cleaned, but it does not(( Maybe leakage because of this: colorCntr= new ColorCounter(); It's never been deleted and I have run-time errors when I try. It is not easy to clean inside a thread. That's why I tried to emit "signal" on "finish()" private slots: void freecolorCntr(){ delete colorCntr; } – avagrkjllkjfdser3 Jul 23 '15 at 22:15
  • I have no idea of QT specifics like the slots, so just guessing here. And I don't understand why you are dynamically allocating all those new and new ColorCounterController objects, even the commented-out code looks like you could use just 1 such object. If you do need many, somehow you seem to delete those objects before they could be much use. – V-R Jul 23 '15 at 23:11
  • Maybe you should get an IDE with a debugger and step through the whole logic having only 1 thread like you said was already working before. Generally avoid dealing with raw pointers in C++: everybody makes mistakes there, myself included. Use shared_ptr / unique_ptr with every "new" and no manual delete's. Use references instead of pointers and std vectors instead of arrays. Sorry, only generic advice here, no silver bullet. – V-R Jul 23 '15 at 23:20
  • 1
    @V-R: " And I don't understand why you are dynamically allocating all those new and new ColorCounterController objects" Might be misapplied technique from Qt. Qt tracks ownership for classes derived from `QObject`/`QWidget`. If object has a specified **owner**, it will be deleted when owner is destroyed. However, I don't see owner being specified for instances `ColorCounter` (this is normally done in constructor) and I since there's no source there's no reason to think that they're derived from `QObject`. – SigTerm Jul 24 '15 at 00:05