HUGE MEMORY LEAK pinpointed!

[rickg22]

[
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.

Yes, but since we're dealing with critical code here, we can't leave anything to luck or past decisions. The code might be changed later, and this line could be forgotten. So, please put the if(!opts.loader) there. It's safe and it doesn't harm.

[dmoore]

ok, and i've added a couple of lines to produce a debug warning about memory leaks if the loader has already been set.

[thomas]

Thomas, what do you think about the patch? Is it OK with you?

I don't understand what either the original code or the patch is doing
In particular, I don't understand what Parse(...., NULL) actually parses, but if it works, that's ok with me

deletes the loaders that aren't complete (this causes seg faults)

Yes, necessarily. The loader object that you get back is aliased in the file manager's queue, so its worker thread knows what to load. If you delete the loader before it has finished loading, FileManager will read from and write to unallocated memory.
This behaviour was accepted when the class was created, as it keeps the class a lot less complicated and it shouldn't be an issue, normally. You don't usually load a dozen files if you don't really want to do anything with them. However, it seems like this assumption may have been wrong.

There are lots of different ways to address this issue - my approach was just the simplest that came to mind.

Probably the best solution will be to take away object ownership from the user. It just doesn't work...

[Der Meister]

There are lots of different ways to address this issue - my approach was just the simplest that came to mind.

Probably the best solution will be to take away object ownership from the user. It just doesn't work...

I agree with that. But that means that the FileLoader is resonsible for freeing the memory. But therefore it has to know whether it is still in use or not required any more. But I see only one way to implement this: smart pointers... Well...

[dmoore]

There are lots of different ways to address this issue - my approach was just the simplest that came to mind.

Probably the best solution will be to take away object ownership from the user. It just doesn't work...

I agree with that. But that means that the FileLoader is resonsible for freeing the memory. But therefore it has to know whether it is still in use or not required any more. But I see only one way to implement this: smart pointers... Well...

is it not possible to use events? when file loading is completed the loader could send a message to the caller saying "here's the data, let me know when you are done (by calling certain member of the loader) so i can clean up"

[Der Meister]

Well, this would again put the ownership of the object to the caller as it has to call this certain member function to clean things up. This isn't much different from requiring the caller to delete the FileLoader as soon as it does not need it any more but requires a much more complicated interface.

[dmoore]

Well, this would again put the ownership of the object to the caller as it has to call this certain member function to clean things up. This isn't much different from requiring the caller to delete the FileLoader as soon as it does not need it any more but requires a much more complicated interface.

If the caller is going to process the data they are going to have to take ownership of it (an auto_ptr may help, of course)

I don't think ownership is that big a problem. IMO, the big problem is that you have to wait until a loader is done loading before you can delete it, but the loader doesn't tell you when its done, so you have create messy threads to wait for it. messaging would be the most consistent way of dealing with this (considering CB is a wxwidgets app)

[killerbot]

can a loader be queried to check it's state wrt finished loading ?

[Der Meister]

I didn't see such a method in its interface but the function to retrieve the data is synchronized with the background-thread (well, what a surprise).
Anyway, I don't see any use from such method. Care to explain?

[killerbot]

stupid as it is, query to know when finished so you can delete it

[dmoore]

IMHO, without messages (or ability to query), threaded fileloaders are pointless because the only way to use them without blocking the main thread is to create another thread (then why do you need the fileloader threaded?)

[thomas]

Committed the following changes:

1. Added a Delete() function to AbstractJob (base class of LoaderBase).
Calling Delete() will:

• discard a job on the queue before anything is allocated, if Delete() is called before the scheduler has seen it
• finish loading but free all allocated memory immediately after, if Delete() is called while FileManger is working on the job
• immediately free all resources if Delete() is called after FileManager has done its work

In other words, you now call loader_ptr->Delete(); rather than delete loader_ptr;. Existing code still works as before.

2. Added Loader as an (untested!) wrapper for LoaderBase*. Basically, Loader can be thought of std::auto_ptr<LoaderBase*> with the exceptions that:

• operator=(LoaderBase*) is implemented
• you can use Loader as LoaderBase for all public user functions, as if operator. was overloaded.

I don't have time to test that class right now, but it should be simple enough to just work... in any case it compiles without errors

So... from now on, we should be able to write code like:

{
LoaderBase* load = FileManager::Load(_T("foo.bar"));
if(strange_condition)
   load->Delete();  // discard, resources will be freed when opportune
}

or

{
Loader load = FileManager::Load(_T("foo.bar"));
wxString x = load.GetData();
} // loader resources freed at end of scope

Obviously, accessing any member on a LoaderBase* after calling Delete() is illegal and may or may not write to unallocated memory.

can a loader be queried to check it's state wrt finished loading ?

Not in trunk.

EDIT:
Test suite for Delete() attached. The patch creates a file that is 1 MB in size and loads it 100 times. Memory consumption is +100 MB temporarily, and +/-0 after calling Delete().
The test has been performed with and without a Sleep(2000) to give FileManager some time to load the data, to the same result.

[attachment deleted by admin]

[thomas]

IMHO, without messages (or ability to query), threaded fileloaders are pointless because the only way to use them without blocking the main thread is to create another thread (then why do you need the fileloader threaded?)

They are not pointless. You tell FileManager that you will be wanting some file (Load()) at some point in the future. FileManager may load it immediately or later, or it might even not do anything at all before you ask for the data.
At some later point, you ask FileManger for a pointer to the data (GetData()) that you want, and FileManager gives you back a valid pointer (or null in case of an error). It is not your problem what FileManager has to do to return that pointer, you can rely that the data will just be there.
Basically, this can be seen as the poor man's implementation of file mapping.

Behind the scenes, FileManager will queue all requests that access local storage into one queue, so to make optimal use of the hardware (keeping the drive as busy as possible, but avoiding multi-thread seek-thrashing), and all requests that involve network into another queue. While the PC is doing mainly DMA or is waiting for the hard drive to seek (or network packets to arrive), the main thread can put CPU cycles into parsing a file, or similar tasks, instead of blocking on fread().

In the example of Code Completion, this parallelism reduces load times from several minutes to a dozen of seconds, as Yiannis has benchmarked a while ago.

[thomas]

I'll remove the Loader wrapper class again, it seems to make more problems than it's worth.

On a different note:
void Parser::BatchParse(const wxArrayString& filenames)
{
    m_batchtimer.Stop();
    m_IsBatch = true;
    m_Pool.BatchBegin();
    for (unsigned int i = 0; i < filenames.GetCount(); ++i)
    {
        LoaderBase* loader = Manager::Get()->GetFileManager()->Load(filenames, false);
        Parse(filenames[ i ], true, loader);
    }

    // Allow future parses to take place in this same run
    m_batchtimer.Start(batch_timer_delay,wxTIMER_ONE_SHOT);
}

Is this ever deleted anywhere? I see that the pointer is added to a ParserThreadOptions struct, but I don't see that it is ever released. This would explain why the memory is never freed.

[dmoore]

this is what the patch addresses (and an identical line in OnParseFile). it moves the Load call into Parse (the 2nd one), and Load only gets called if a ParserThread is created (that classes destructor releases the loader object). Not only does that avoid memory leaks, but it also saves hundreds of unnecessary loads (part of the disk thrashing?)