Developer forums (C::B DEVELOPMENT STRICTLY!) > Development

HUGE MEMORY LEAK pinpointed!

<< < (15/21) > >>

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



--- Quote from: killerbot on June 24, 2007, 05:32:32 pm ---can a loader be queried to check it's state wrt finished loading ?
--- End quote ---
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:

--- Quote from: dmoore on June 24, 2007, 06:30:07 pm ---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?)
--- End quote ---
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?)

Navigation

[0] Message Index

[#] Next page

[*] Previous page