Cannot understand this memory leak.

S

Simon

Hi,

I have some old code that I am trying to use in my current project.
But my compiler is reporting a memory leak. (it does not tell me where the
leak comes from).

I have narrowed down the code as follow where the error could be.

// --
// in the header
std::string *m_pData;

//-------
// in the constructor
m_pData = new std::string[ someKnownSize + 1]
memset( m_pData, 0, (n+1)*sizeof(std::string) );
...

//-------
// The destructor
if( NULL != m_pData )
delete [] ( m_pData );
m_pData = NULL;
m_nSize = 0;


//-------
// in a function
void add( std::string &s, int len )
{
m_pData[m_nSize++] = std::string(s, len ) ;
}

//-------

if I remove the line "m_pData[m_nSize++] = std::string(s, len ) ;" then I
have no leak.

What could be the problem and how can I find the leak?

Many thanks

Simon
 
V

Victor Bazarov

Simon said:
Hi,

I have some old code that I am trying to use in my current project.
But my compiler is reporting a memory leak. (it does not tell me
where the leak comes from).

I have narrowed down the code as follow where the error could be.

// --
// in the header
std::string *m_pData;

//-------
// in the constructor
m_pData = new std::string[ someKnownSize + 1]
memset( m_pData, 0, (n+1)*sizeof(std::string) );
^^^^^^^^^^^^^^^^
Throw this 'memset' away (comment it out). You shouldn't do it
at all.
..

//-------
// The destructor
if( NULL != m_pData )
delete [] ( m_pData );

There is no need to check for NULL.
m_pData = NULL;
m_nSize = 0;

Setting _your own_ data member values in the destructor is
pointless, they are going away.
//-------
// in a function
void add( std::string &s, int len )
{
m_pData[m_nSize++] = std::string(s, len ) ;
}

//-------

if I remove the line "m_pData[m_nSize++] = std::string(s, len ) ;"
then I have no leak.

What could be the problem and how can I find the leak?

There does not seem to be the reason for any leak, but your library
implementation may be buggy or your tool is reporting false positives
on the leaks.

V
 
T

Tristan Wibberley

On Wed, 2008-01-23 at 21:51 +0200, Simon wrote:

[C-style code with bug reminiscent of the days before I started using C++ snipped]

I suggest this instead:

// --
// in the header
std::vector<std::string> data;

//-------
// in the constructor
data.reserve(someKnownSize + 1);
..

//-------
// The destructor
// GONE! Yay! all simple

//-------
// in a function
void add( std::string &s, int len )
{
data.push_back(std::string(s, len )) ;
}

//-------

--
Tristan Wibberley

Any opinion expressed is mine (or else I'm playing devils advocate for
the sake of a good argument). My employer had nothing to do with this
communication.
 
J

James Kanze

Simon said:
I have some old code that I am trying to use in my current project.
But my compiler is reporting a memory leak. (it does not tell me
where the leak comes from).
I have narrowed down the code as follow where the error could be.
// --
// in the header
std::string *m_pData;
//-------
// in the constructor
m_pData = new std::string[ someKnownSize + 1]
memset( m_pData, 0, (n+1)*sizeof(std::string) );
^^^^^^^^^^^^^^^^
Throw this 'memset' away (comment it out). You shouldn't do it
at all.

Doing has just introduced undefined behavior into his code.
He's zapping the memory of fully constructed strings, which is a
sure recipe for disaster.
..
//-------
// The destructor
if( NULL != m_pData )
delete [] ( m_pData );
There is no need to check for NULL.

Especially as he knows its not null, having set it in the
constructor.
Setting _your own_ data member values in the destructor is
pointless, they are going away.
//-------
// in a function
void add( std::string &s, int len )
{
m_pData[m_nSize++] = std::string(s, len ) ;
}
//-------
if I remove the line "m_pData[m_nSize++] = std::string(s, len ) ;"
then I have no leak.
What could be the problem and how can I find the leak?
There does not seem to be the reason for any leak, but your
library implementation may be buggy or your tool is reporting
false positives on the leaks.

If the default constructor for std::string allocates memory,
there's a good chance that it got lost when he memset. If
m_nSize in add ever gets too big, he's also writing to unknown
memory. If by chance, the default constructor for string does
leave all zeros, and that memory happens to contain all zeros,
then the assignment is likely to work, but of course, the
destructor of that string won't be called in his delete
statement.

The code is bad. He really should replace it with a simple
std::vector< std::string >, using push_back in his add()
function.
 
S

Simon

// --
// in the header
std::vector<std::string> data;

//-------
// in the constructor
data.reserve(someKnownSize + 1);
..

//-------
// The destructor
// GONE! Yay! all simple

//-------
// in a function
void add( std::string &s, int len )
{
data.push_back(std::string(s, len )) ;
}

//-------


Thanks, doing it this way is far easier and does work.
As mentioned the memset was probably the problem anyway.

The reason why I did this in the first place was because I was told that
push_back() was very time consuming.

As I know the size I was hopping that declaring an array would buy me a few
ticks.

do you think that using push_back is the fastest way I can fill my array?

Thanks again.

Simon
 
V

Victor Bazarov

Simon said:
Thanks, doing it this way is far easier and does work.
As mentioned the memset was probably the problem anyway.

The reason why I did this in the first place was because I was told
that push_back() was very time consuming.

As I know the size I was hopping that declaring an array would buy me
a few ticks.

do you think that using push_back is the fastest way I can fill my
array?

'push_back' is relatively fast in filling the pre-allocated vector
(for which you did 'reserve'), since it does not require any
reallocations. If you're concerned, profile your application to
see how much time is spent in 'push_back' and then decide if it is
acceptable.

V
 
P

Puppet_Sock

The reason why I did this in the first place was because I was told that
push_back() was very time consuming.

As I know the size I was hopping that declaring an array would buy me a few
ticks.

Do you in fact have some kind of spec on how fast
your program has to be? If not, why are you wasting
your time trying to optimize it for speed?

Get it right. Then get it fast. Don't worry about
omtimizing on such little issues as this until
you actually have something like a working code.
Then if it fails to meet the performance spec,
go looking for optimizations. And be sure to take
a stop watch with you to determine if a given
optimization in fact makes any appreciable change.
do you think that using push_back is the fastest way I can fill my array?

I think that you need to worry about other things
before you worry about doing things the fastest
way you can.
Socks
 
S

Simon

The reason why I did this in the first place was because I was told that
Do you in fact have some kind of spec on how fast
your program has to be? If not, why are you wasting
your time trying to optimize it for speed?

Well the files are text files loaded at run time, (once with some kind of
progress bar).
The faster the file is loaded the better.

Currently it takes between 10 to 20 seconds on a 'normal/average' machine.

Ideally I would convert the file so it is loaded in my array as fast as
possible.
It is all about user experience.
Get it right. Then get it fast. Don't worry about
omtimizing on such little issues as this until
you actually have something like a working code.

The code is working, because I am reading 40000 lines I am hopping that any
tick I can shave off will buy me a few seconds in the long run.
Then if it fails to meet the performance spec,
go looking for optimizations. And be sure to take
a stop watch with you to determine if a given
optimization in fact makes any appreciable change.

The specs are already met, loading the file.
But I want to load the file as fast as possible to make the user experience
even more enjoyable.
I think that you need to worry about other things
before you worry about doing things the fastest
way you can.

Well, it does work now.
I can use the program as it is now, but the faster will make the user
experience more enjoyable.
Yes I could tell them to wait 30 seconds, but waiting 10 secs would be even
better.

Simon
 
J

Jerry Coffin

[ ... ]
Well the files are text files loaded at run time, (once with some kind of
progress bar). The faster the file is loaded the better.

Currently it takes between 10 to 20 seconds on a 'normal/average' machine.

My guess is that roughly 99.999% of this is time to read the disk, and
what you do with the data after you read it won't make any noticeable
difference.
Ideally I would convert the file so it is loaded in my array as fast as
possible.
It is all about user experience.

The time to resize a vector is measured in microseconds -- far below
what a user can hope to detect.
The code is working, because I am reading 40000 lines I am hopping that any
tick I can shave off will buy me a few seconds in the long run.

What you're doing right now is about equivalent to somebody who's
grossly obese trying to lose weight by trimming his fingernails. You
need to look at the time to actually read the data, not the time taken
by the processor to store the data after it's been read. Chances are
good that maximum performance will require non-portable code, such as
memory mapping the file, or doing some of the reading in the background,
so you can start doing some processing before you've finished reading
the file.
 
S

Simon

[ ... ]
Well the files are text files loaded at run time, (once with some kind of
progress bar). The faster the file is loaded the better.

Currently it takes between 10 to 20 seconds on a 'normal/average'
machine.

My guess is that roughly 99.999% of this is time to read the disk, and
what you do with the data after you read it won't make any noticeable
difference.

I read the entire file first into memory, then I read each 'lines'
The time to resize a vector is measured in microseconds -- far below
what a user can hope to detect.


What you're doing right now is about equivalent to somebody who's
grossly obese trying to lose weight by trimming his fingernails.

Maybe, but when will I be able to calculate that I am going as fast as the
system can go?
When will fast enough be enough.

Because, if I get it down to 5 seconds, the user might want it down to 1
second and so on.
need to look at the time to actually read the data,

Less than half a second.
I use the file size, (fseek (hf , 0 , SEEK_END); + ftell)

And then I read the whole thing into a char *

Simon
 
J

Jerry Coffin

[ ... ]
Maybe, but when will I be able to calculate that I am going as fast as the
system can go?
When will fast enough be enough.

The usual in this direction is to look at the specifications of the
hardware. If you look carefully through the specs for most disk drives,
you can find a specification of how fast it can transfer data to/from
the platters. This is a hard limit, so you can't possibly go faster than
that except under special circumstances such as the data already being
in the cache.

[ ... ]
Less than half a second.
I use the file size, (fseek (hf , 0 , SEEK_END); + ftell)

And then I read the whole thing into a char *

Something here doesn't add up. It's almost impossible to say what since
you don't seem to have posted the actual code, but if you're able to
read the data from the disk in less than half a second, you definitely
should NOT need anywhere close to "10 to 20 seconds" to break that up
into lines, store it in a vector, or _almost_ anything else you feel
like doing.
 
M

Matthias Buelow

Jerry said:
Something here doesn't add up. It's almost impossible to say what since
you don't seem to have posted the actual code, but if you're able to
read the data from the disk in less than half a second, you definitely
should NOT need anywhere close to "10 to 20 seconds" to break that up
into lines, store it in a vector, or _almost_ anything else you feel
like doing.

Perhaps he's reading a large file into memory, and then essentially
copies the stuff, exhausting the physical RAM and the machine starts
swapping madly. Hard to tell without any further details, of course.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,743
Messages
2,569,478
Members
44,899
Latest member
RodneyMcAu

Latest Threads

Top