memory leak in class code

[florian kno]

hello all!

i'm using gcc 3.2 on linux.
i have a (self developed) matrix class:

class cMatrix { // unneccessary info stripped
....
double *data; // matrix data is stored in a heap array

cMatrix row(const int& r) const;
// const cMatrix& row(const int& r) const; // is this better?? why?
....
const double& dotproduct(const cMatrix& m1, const cMatrix& m2) const;
....
}

cMatrix cMatrix::row(const int& r) const {
cMatrix *tmp=new cMatrix(1,ncols);
for(int i=0;i<ncols;i++) (*tmp)=data[r*ncols + i];
return *tmp;
}

const double& cMatrix::dotproduct(const cMatrix& v1, const cMatrix& v2)
const {
double d=0.0;
for(int i=0;i<v1.elements();i++) d+=v1*v2;
return d;
}


int main() {

{
cMatrix m(10,10);
cMatrix r=m.row(1);
}

return 0;
}

using valgrind i found out that the above main() leaks memory.

my questions are:
* how to alter the code of cMatrix::row to avoid memory leaks?
* how to avoid objects stored on the stack that get overriden later
when program code is further executed?
* is there a better syntax for (*tmp)="some value"?

additional info:
* i later definitly need something like
res=dotproduct(Ni, nodes.column(i)); (where Ni is also a matrix)
to work correctly.(^^^^^^^^^^^^^^^ temporary object)
-> operator and method chaining.

thanks for any help or ideas!

 

[Peter van Merkerk]

hello all!

i'm using gcc 3.2 on linux.
i have a (self developed) matrix class:

class cMatrix { // unneccessary info stripped
...
double *data; // matrix data is stored in a heap array

cMatrix row(const int& r) const;
// const cMatrix& row(const int& r) const; // is this better?? why?
...
const double& dotproduct(const cMatrix& m1, const cMatrix& m2) const;
...
}

cMatrix cMatrix::row(const int& r) const {
cMatrix *tmp=new cMatrix(1,ncols);
for(int i=0;i<ncols;i++) (*tmp)=data[r*ncols + i];
return *tmp;
}

const double& cMatrix::dotproduct(const cMatrix& v1, const cMatrix& v2)
const {
double d=0.0;
for(int i=0;i<v1.elements();i++) d+=v1*v2;
return d;
}


int main() {

{
cMatrix m(10,10);
cMatrix r=m.row(1);
}

return 0;
}

using valgrind i found out that the above main() leaks memory.

my questions are:
* how to alter the code of cMatrix::row to avoid memory leaks?

Don't use 'new' when you don't have to:

cMatrix cMatrix::row(const int& r) const
{
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++)
{
tmp = data[r*ncols + i];
}
return tmp;
}

In C++ you don't have to use 'new' to instatiate class objects.

* how to avoid objects stored on the stack that get overriden later
when program code is further executed?

I don't understand this question???

* is there a better syntax for (*tmp)="some value"?

If you don't use pointers like in the (untested) code I showed you, you
don't get the ugly syntax.

additional info:
* i later definitly need something like
res=dotproduct(Ni, nodes.column(i)); (where Ni is also a matrix)
to work correctly.(^^^^^^^^^^^^^^^ temporary object)
-> operator and method chaining.

I don't see your problem here.

 

[florian kno]

hello!

my questions are:
* how to alter the code of cMatrix::row to avoid memory leaks?

Don't use 'new' when you don't have to:

cMatrix cMatrix::row(const int& r) const
{
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++)
{
tmp = data[r*ncols + i];
}
return tmp;
}

In C++ you don't have to use 'new' to instatiate class objects.
ok.

* how to avoid objects stored on the stack that get overriden later
when program code is further executed?

I don't understand this question???

i was thinking that the local objects are stored on the stack (that's why i
created new ones not on the stack) and that a reference is returned to
memory space that is later overwritten.

I don't see your problem here.

that's somewhat redundant with my stack problem.

thanks.

 

[Peter van Merkerk]

Don't use 'new' when you don't have to:

cMatrix cMatrix::row(const int& r) const
{
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++)
{
tmp = data[r*ncols + i];
}
return tmp;
}

In C++ you don't have to use 'new' to instatiate class objects. ok.

* how to avoid objects stored on the stack that get overriden later
when program code is further executed?

I don't understand this question???

i was thinking that the local objects are stored on the stack (that's why i
created new ones not on the stack) and that a reference is returned to
memory space that is later overwritten.

It appears to me that you have a Java mindset. If that is the case keep
telling youself C++ is not Java, even though the syntax looks similar.
Java has reference semantics, C++ has value semantics. This means that
when the function cMatrix::row() returns the tmp object will be copied
into the object that receives the return value (you did implement a copy
constructor, did you?). After the tmp object has been copied, it will be
destroyed. A smart optimizer may be able to optimize the copying away,
but that should be of no concern to you.

Your concern would be valid if cMatrix::row() would return references
(cMatrix&) to local objects. An alternative for your orignal
implementation of the cMatrix::row() function would to let it return a
pointer (cMatrix*), and put the burden of deleting the object on the
caller. It avoids copying of the matrix but generally you want to avoid
this kind of construct, as it is inconvenient for the caller and it may
easilly lead to memory leaks.

 

[florian kno]

Peter van Merkerk wrote:

It appears to me that you have a Java mindset. If that is the case keep

yes, i have a java background. using your hints i have been able to reduce
my memory leaks from 80 to 3. and these 3 will be killed shortly.

the result of my program is still the same so things work.

telling youself C++ is not Java, even though the syntax looks similar.
Java has reference semantics, C++ has value semantics. This means that
when the function cMatrix::row() returns the tmp object will be copied
into the object that receives the return value (you did implement a copy
constructor, did you?).

yes, i have.

After the tmp object has been copied, it will be
destroyed. A smart optimizer may be able to optimize the copying away,
but that should be of no concern to you.

Your concern would be valid if cMatrix::row() would return references
(cMatrix&) to local objects. An alternative for your orignal
implementation of the cMatrix::row() function would to let it return a
pointer (cMatrix*), and put the burden of deleting the object on the
caller. It avoids copying of the matrix but generally you want to avoid
this kind of construct, as it is inconvenient for the caller and it may
easilly lead to memory leaks.

later, when my program continues to grow, i want to do (actually i have to):

result=(matrix1.row(4).crossproduct(matrix2.column(2)).transpose()+matrixtemp
).dotproduct(matrix3.inverse()); // something written here

)

thanks for your help!

 

[Jesper Madsen]

Did you implement a destructor?
And did you do a assignment operator and a copy constructor???
If you pass this cMatrix around, i believe you need it

 

[florian kno]

Did you implement a destructor?
And did you do a assignment operator and a copy constructor???
If you pass this cMatrix around, i believe you need it

actually i have all of the above mentioned methods.
problem was solved by skipping the pointer stuff (*).

cMatrix cMatrix::row(const int& r) const {
cMatrix *tmp=new cMatrix(1,ncols);
for(int i=0;i<ncols;i++) (*tmp)=data[r*ncols + i];
return *tmp;
}

is now:

const cMatrix cMatrix::row(const int& r) const {
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++) tmp=data[r*ncols + i];
return tmp;
}

 

[Peter van Merkerk]

cMatrix cMatrix::row(const int& r) const {
cMatrix *tmp=new cMatrix(1,ncols);
for(int i=0;i<ncols;i++) (*tmp)=data[r*ncols + i];
return *tmp;
}

is now:

const cMatrix cMatrix::row(const int& r) const {
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++) tmp=data[r*ncols + i];
return tmp;
}

If you return by value like in this case, there is no point in putting const
before cMatrix. It doesn't do any harm, but it doesn't do any good either.
Since the caller gets a copy of the matrix generated by cMatrix::row(), it
doesn't matter what the caller does with the copy as it won't affect the
object that produced the copy.

If you would return by reference (not a viable option in this example), or
pass values by reference you should use const whenever possible. For
primitive types like 'int' there is usually no benefit in passing them by
const reference because copying is extremely fast for these types. Passing
them by value is often just as fast (if not faster), and results in clearer
code. For objects whose copy constructor may take some time (your cMatrix
class is an example of this) it is advisable to pass them by const
references.

 

[Big Brian]

Did you implement a destructor?

And did you do a assignment operator and a copy constructor???
If you pass this cMatrix around, i believe you need it

actually i have all of the above mentioned methods.
problem was solved by skipping the pointer stuff (*).

cMatrix cMatrix::row(const int& r) const {
cMatrix *tmp=new cMatrix(1,ncols);
for(int i=0;i<ncols;i++) (*tmp)=data[r*ncols + i];
return *tmp;
}

is now:

const cMatrix cMatrix::row(const int& r) const {
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++) tmp=data[r*ncols + i];
return tmp;
}

This solves the memory leak, however you might not want to do this if
your matrix is big since it creates a temporary object, and copies the
data twice. You may not want to do for perfance and resource issues,
if your matrix is large. Oh I just re-read the original code, and
since it was returning *tmp, it has the same issue.

I have no idea how big your matrices are going to be, but if they're
going to be large, you may want to think about another solution.
Since your row function returns a const cMatrix, you could (maybe
somehow) return a pointer into "data", and thus not have to copy it,
which you're doing twice in your current code.

const cMatrix cMatrix::row(const int& r) const {
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++){
//COPYING THE DATA HERE...
tmp=data[r*ncols + i];
}
return tmp;
}

and when this is used...

const cMatrix m = (someobject).row(x); // copying the temp object
here, ie copying the data a second time

Maybe it's not an issue, but maybe it is too.

-Brian

 

[Peter van Merkerk]

is now:

const cMatrix cMatrix::row(const int& r) const {
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++) tmp=data[r*ncols + i];
return tmp;
}

This solves the memory leak, however you might not want to do this if
your matrix is big since it creates a temporary object, and copies the
data twice. You may not want to do for perfance and resource issues,
if your matrix is large. Oh I just re-read the original code, and
since it was returning *tmp, it has the same issue.

The best approach is to make right first, and make it fast later.

I have no idea how big your matrices are going to be, but if they're
going to be large, you may want to think about another solution.
Since your row function returns a const cMatrix, you could (maybe
somehow) return a pointer into "data", and thus not have to copy it,
which you're doing twice in your current code.

A good optimizer (one that supports return value optimization) might be able
to optimize the redundant copying away. If that is the case returning
pointers would be no faster. The problem with returning pointers is that it
is very inconvient for the caller. For example try transforming this to a
version with pointers that doesn't leak:


result=(matrix1.row(4).crossproduct(matrix2.column(2)).transpose()+matrixtem
p).dotproduct(matrix3.inverse());

Yes, it is possible to do that. But it also terribly inconvenient; it is
extremely easy for the caller to leak memory, it would take many more lines
than the one without pointers and the resulting code would be a lot less
readable.

const cMatrix cMatrix::row(const int& r) const {
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++){
file://COPYING THE DATA HERE...
tmp=data[r*ncols + i];
}
return tmp;
}

and when this is used...

const cMatrix m = (someobject).row(x); // copying the temp object
here, ie copying the data a second time

Maybe it's not an issue, but maybe it is too.

If it is an issue (and it may very well be so when dealing with large
matrixes and less than perfect optimizers), an alternative could be to use a
proxy object. This technique has been used often used with string
implementations. The advantage of this technique is that it transparent for
the caller and that it puts the burden on the implementor(s) of the cMatrix
class rather than its users. But this technique has downsides too and it is
easy to get it wrong leading to very obscure bugs.

But like I said before first get it right. If it is too slow, use a profiler
to pinpoint the performance bottlenecks. If it is the copy constructor then
you may consider the techniques discussed here. OTOH the performance
bottleneck might be just as well somewhere else (matrix computations tend to
burn plenty of CPU cycles as well). Anyway I assume this is just an
excersise for the OP. There are plenty of C++ Matrix libraries around, so if
it is a real project it would be more logical to look at those first.

 

[florian kno]

first, thanks for all help here.

The best approach is to make right first, and make it fast later.

that's what i do.

a pointer into data would be possible only in the case of row. if i use
column, what i also have to, things will be more complicated. i would have
to generate a new matrix... (at least in the first point)

A good optimizer (one that supports return value optimization) might be
able to optimize the redundant copying away. If that is the case returning
pointers would be no faster.

when my program is finished i will dive into optimization and look at the
assembly code of my weakest parts a bit and try to make things better
there.

using -O2 instead of -O0 (gcc optimisation level 2) devided the runtime into
half so i think i could do a lot... but first my prog should work
completly.

The problem with returning pointers is that
it is very inconvient for the caller. For example try transforming this to
a version with pointers that doesn't leak:

Yes, it is possible to do that. But it also terribly inconvenient; it is
extremely easy for the caller to leak memory, it would take many more
lines than the one without pointers and the resulting code would be a lot
less readable.

readablity is a bit issue here since other people will relay on my results
and will propably reuse code.

But like I said before first get it right. If it is too slow, use a
profiler to pinpoint the performance bottlenecks. If it is the copy
constructor then you may consider the techniques discussed here. OTOH the
performance bottleneck might be just as well somewhere else (matrix
computations tend to burn plenty of CPU cycles as well).

the performance bottleneck is indeed somewhere else. its the computation of
the values that get filled in the *big* matrix. and eventually the
inversion of that big matrix.

the row stuff is only for extracting 3 coordinate values of an array of
points.

Anyway I assume
this is just an excersise for the OP. There are plenty of C++ Matrix
libraries around, so if it is a real project it would be more logical to
look at those first.

hmm, no, it is my diploma work for university (sort of phd?). and i'm no
computer programmer. at least i'm not experienced.
one goal of my program is to only use c/c++ and stl.
acutally i look up code from mtl (=matrix template libary) and books
(fortran-code, where my problem is partly described somehow).