Author Topic: HUGE MEMORY LEAK pinpointed!  (Read 63221 times)

Offline Der Meister

  • Regular
  • ***
  • Posts: 307
Re: HUGE MEMORY LEAK pinpointed!
« Reply #45 on: June 22, 2007, 06:00:55 pm »
Bad news: Just putting a delete[] data; in in front of data = new char[len+1]; didn't solve the memory leak. I don't know if it changed anything as I didn't save the ouput of the valgrind run before this patch - shame on me. But still, just starting Code::Blocks, opening codeblocks.cbp, waiting until CC-plugin finished parsing and closing Code::Blocks left about 10MB non-freed memory behind just because of this function. According to this I would guess that the destrcutor of LoaderBase never gets called.
Real Programmers don't comment their code. If it was hard to write, it should be hard to understand.
Real Programmers don't write in BASIC. Actually, no programmers write in BASIC, after the age of 12.

Offline rickg22

  • Lives here!
  • ****
  • Posts: 2283
Re: HUGE MEMORY LEAK pinpointed!
« Reply #46 on: June 22, 2007, 06:12:44 pm »
Der Meister: Try doing the following experiment.

Open and close the Code::Blocks project several times, open it again and close the app. That should leave us with 70 or so megabytes of data floating around. That'll be much easier to locate in the valgrind logs.

Offline Biplab

  • Developer
  • Lives here!
  • *****
  • Posts: 1874
    • Biplab's Blog
Re: HUGE MEMORY LEAK pinpointed!
« Reply #47 on: June 22, 2007, 06:20:08 pm »
Der Meister: Try doing the following experiment.

Open and close the Code::Blocks project several times, open it again and close the app. That should leave us with 70 or so megabytes of data floating around. That'll be much easier to locate in the valgrind logs.

Repeating valgrind tests can be painful. ;)

Valgrind runs the app approx. 10-20x times slower. I ran that test in a Virtual Machine. So it took a lot of time to get one complete log.
Be a part of the solution, not a part of the problem.

Offline Der Meister

  • Regular
  • ***
  • Posts: 307
Re: HUGE MEMORY LEAK pinpointed!
« Reply #48 on: June 22, 2007, 06:32:34 pm »
No, I really don't want to do this. As Biplab said, valgrind runs are painful.

Anyway, I *hope* I found the reason for this. TDragon's post pointed me to the right direction: The BackgroundThread instance does not delete the jobs as it does not own them. Therefore the caller of FileManager::Load has to free the memory but it looks like this was never done. I just pachted a few places and will start another valgrind run to check if it works.
Real Programmers don't comment their code. If it was hard to write, it should be hard to understand.
Real Programmers don't write in BASIC. Actually, no programmers write in BASIC, after the age of 12.

Offline thomas

  • Administrator
  • Lives here!
  • *****
  • Posts: 3979
Re: HUGE MEMORY LEAK pinpointed!
« Reply #49 on: June 22, 2007, 06:42:18 pm »
There's the culprit! According to the report, there is a new[] without delete[] in FileLoader::operator()().
No, this is just like it has to be.

There's a Delete( ) operation done.
But I can't find ANYWHERE the function "Delete" (with uppercase).
It is a function that has been in globals.h for many months, and which is now in prep.h (so it's available everywhere).
What it does is, it calls the C++ delete operator on a pointer-to-object and (within thread scope) atomically zeroes the pointer.

I added this many months ago in response to the crashes that were caused by code like

if(pointer)
   delete pointer;


...which does not only contain an unnecessary if() but which was also the reason for regular mysterious crashes when a pointer was not set to zero after being delted, and subsequently was deleted a second time.

Except for member pointers that are deleted in class destructors, Delete() should always be used in preference to delete.
Using Delete() in a class destructor is of course possible, but since the members are invalid after this, there is unnecessary overhead in zeroing the pointer.
"We should forget about small efficiencies, say about 97% of the time: Premature quotation is the root of public humiliation."

Offline Der Meister

  • Regular
  • ***
  • Posts: 307
Re: HUGE MEMORY LEAK pinpointed!
« Reply #50 on: June 22, 2007, 07:36:27 pm »
Again bad news: I spotted only one location with a call to FileManager::Load without freeing the reserved memory:
- src/app.cpp:512

But adding this delete did not solve our big memory lead. Perhaps a small one, but not our real one. I have no idea where there is a call to delete missing. *Maybe* somewhere in the CC-plugin but I didn't find it. Anyway, if I did not overlook something the CC-plugin delegates the ownership of this objects to the cbThreadPool. I did not understand this class completely but it has calls to delete but I'm not sure if they are really used. There is at least one in the destructor of the thread pool which should free all memory but this one would not solve our problem as it would only free the memory when Code::Blocks is closed. But we have to free the memory after the CC-plugin finished its parsing.


Edit: Found two minor possible memory leaks. Shouldn't there be deletes in the following two functions?
Code
bool Parser::ParseBufferForFunctions(const wxString& buffer)
{
    ParserThreadOptions opts;
    opts.wantPreprocessor = m_Options.wantPreprocessor;
    opts.useBuffer = true;
    opts.bufferSkipBlocks = true;
    opts.handleFunctions = true;
    ParserThread* thread = new ParserThread(this,
                                            buffer,
                                            false,
                                            opts,
                                            m_pTempTokens);
    return thread->Parse();
}

bool Parser::ParseBufferForUsingNamespace(const wxString& buffer, wxArrayString& result)
{
    ParserThreadOptions opts;
//    opts.wantPreprocessor = m_Options.wantPreprocessor; // ignored
//    opts.useBuffer = false; // ignored
//    opts.bufferSkipBlocks = false; // ignored
    ParserThread* thread = new ParserThread(this,
                                            wxEmptyString,
                                            false,
                                            opts,
                                            m_pTempTokens);
    return thread->ParseBufferForUsingNamespace(buffer, result);
}
« Last Edit: June 22, 2007, 08:05:35 pm by Der Meister »
Real Programmers don't comment their code. If it was hard to write, it should be hard to understand.
Real Programmers don't write in BASIC. Actually, no programmers write in BASIC, after the age of 12.

Offline rickg22

  • Lives here!
  • ****
  • Posts: 2283
Re: HUGE MEMORY LEAK pinpointed!
« Reply #51 on: June 22, 2007, 08:51:45 pm »
Yes, you're right about that one. Still small, but better than nothing.

However I still have the impression that those memory leaks must be found elsewhere. This is quite a sleuth case...  :?

Offline Der Meister

  • Regular
  • ***
  • Posts: 307
Re: HUGE MEMORY LEAK pinpointed!
« Reply #52 on: June 22, 2007, 09:00:36 pm »
Yes, they really should be found and fixed. But digging throw the CC-plugin isn't really easy and I didn't find a problem yet.
Real Programmers don't comment their code. If it was hard to write, it should be hard to understand.
Real Programmers don't write in BASIC. Actually, no programmers write in BASIC, after the age of 12.

Offline dmoore

  • Developer
  • Lives here!
  • *****
  • Posts: 1576
Re: HUGE MEMORY LEAK pinpointed!
« Reply #53 on: June 23, 2007, 03:13:25 pm »
i ran some quick constructor/destructor counting tests and confirmed file loaders weren't being released as per valgrind logs. with CC enabled, this lead to thousands of open file loaders being left behind after opening and closing the CB main project a couple of times. I think similar problems could plague other plugins if they use the loader (filemanager seems to require callers to release loaders, but only after they have finished loading??)

hacky patch here: http://developer.berlios.de/patch/index.php?func=detailpatch&patch_id=2075&group_id=5358

please test. I think the problem boils down to the parser code only releasing file loaders if the thread is actually parsed (not all files will get the ok for parsing). i changed the code to only start a file loader if parsing is actually required. the affected functions are onparsefile and batchparse and bool Parser::Parse(const wxString& bufferOrFilename, bool isLocal, ParserThreadOptions& opts)

PS: the cc code is spaghetti with (IMHO) an unnecessarily complex threading model.
« Last Edit: June 23, 2007, 04:20:00 pm by dmoore »

Offline Der Meister

  • Regular
  • ***
  • Posts: 307
Re: HUGE MEMORY LEAK pinpointed!
« Reply #54 on: June 23, 2007, 07:09:15 pm »
Thank you for the patch, it seems to work quite well. Just compiled Code::Blocks with it and did a short test within valgrind. I didn't look through the whole output but the big leak from the FileLoader seems to be gone.  8)
Real Programmers don't comment their code. If it was hard to write, it should be hard to understand.
Real Programmers don't write in BASIC. Actually, no programmers write in BASIC, after the age of 12.

Offline rickg22

  • Lives here!
  • ****
  • Posts: 2283
Re: HUGE MEMORY LEAK pinpointed!
« Reply #55 on: June 23, 2007, 08:05:29 pm »
PS: the cc code is spaghetti with (IMHO) an unnecessarily complex threading model.

The complex threading model was necessary, trust us. Parsing 1,000 files in the main thread is simply out of the question.
But I agree 100% with you on the spaghetti. I'm researching and testing a new model for Code Completion to implement after v1.0 is released. It's a longterm project.

Offline rickg22

  • Lives here!
  • ****
  • Posts: 2283
Re: HUGE MEMORY LEAK pinpointed!
« Reply #56 on: June 23, 2007, 08:11:55 pm »
hacky patch here: http://developer.berlios.de/patch/index.php?func=detailpatch&patch_id=2075&group_id=5358

The patch has an error.

Code
opts.loader=Manager::Get()->GetFileManager()->Load(bufferOrFilename, false);

Should be:

Code
if(!opts.loader)
{
    opts.loader=Manager::Get()->GetFileManager()->Load(bufferOrFilename, false);
}

Otherwise you'd be overwriting the loader in case the function gets called somewhere else. Please fix and resend the patch :)

Offline thomas

  • Administrator
  • Lives here!
  • *****
  • Posts: 3979
Re: HUGE MEMORY LEAK pinpointed!
« Reply #57 on: June 24, 2007, 01:42:09 am »
(filemanager seems to require callers to release loaders, but only after they have finished loading??)
Yes, and yes. However, in the case of deleting loaders before they have finished loading, the memory leak is the least of your problems, since you're referencing unallocated memory all the way.
This is a well-known behaviour of FileManager that was discussed when it was first created.

I haven't looked into the code completion code, but if it does something like open many files and delete the loaders right away, then this sure explains why memory is leaked.
"We should forget about small efficiencies, say about 97% of the time: Premature quotation is the root of public humiliation."

Offline rickg22

  • Lives here!
  • ****
  • Posts: 2283
Re: HUGE MEMORY LEAK pinpointed!
« Reply #58 on: June 24, 2007, 02:00:50 am »
Thomas, what do you think about the patch? Is it OK with you?

Offline dmoore

  • Developer
  • Lives here!
  • *****
  • Posts: 1576
Re: HUGE MEMORY LEAK pinpointed!
« Reply #59 on: June 24, 2007, 03:15:36 am »
The patch has an error.

Code
opts.loader=Manager::Get()->GetFileManager()->Load(bufferOrFilename, false);

Should be:

Code
if(!opts.loader)
{
    opts.loader=Manager::Get()->GetFileManager()->Load(bufferOrFilename, false);
}

Otherwise you'd be overwriting the loader in case the function gets called somewhere else. Please fix and resend the patch :)

in my patch, I NULLed out the loader member of opts in all instances where Parse is called and needs a loader, so (!opts.loader) should always be true. More importantly, in the present state of the plugin, the loader should never be called (using Load) outside of Parse or there will be memory leaks, so loader must always be NULL unless you come up with a more elaborate patch.

(filemanager seems to require callers to release loaders, but only after they have finished loading??)
Yes, and yes. However, in the case of deleting loaders before they have finished loading, the memory leak is the least of your problems, since you're referencing unallocated memory all the way.
This is a well-known behaviour of FileManager that was discussed when it was first created.

I haven't looked into the code completion code, but if it does something like open many files and delete the loaders right away, then this sure explains why memory is leaked.

No, CC never deletes the loaders that aren't complete (this causes seg faults), it just fails to delete some loaders and loaders don't delete themselves because they have to store the file data for the caller to use. There are lots of different ways to address this issue - my approach was just the simplest that came to mind.

The complex threading model was necessary, trust us. Parsing 1,000 files in the main thread is simply out of the question.
But I agree 100% with you on the spaghetti. I'm researching and testing a new model for Code Completion to implement after v1.0 is released. It's a longterm project.

I wasn't claiming that you could parse files on the main thread, I was just claiming the threading could be a lot simpler, which would make it more maintainable. the actual parser code, by comparison, looks quite readable.