Memory Leak in Array<T> class

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Memory Leak in Array<T> class

David Bateman-3
All of the void constructors derived from Array<T> have a memory leak. The
bit of the class that describes the problem is

template <class T>
class
Array
{
protected:
  class ArrayRep
  {
  public:
    ArrayRep (void) : data (0), len (0), count (1) { }
  };
private:

  typename Array<T>::ArrayRep *nil_rep (void) const
    {
      static typename Array<T>::ArrayRep *nr
        = new typename Array<T>::ArrayRep ();

      return nr;
    }

  Array (void)
    : rep (nil_rep ()), dimensions (),
      idx (0), idx_count (0) { rep->count++; }
};

So what happens is that the void constructor calls nil_rep which creates
the internal representation and adds one to the count. A new ArrayRep
is created for each void constructor. However the constructor itself
then adds 1 to rep->count and the ArrayRep therefore becomes immortal
as its count can never reach 1, when the Array<T>::~Array destructor
is called. This can easily be demostrated with the code snippet.

#include <octave/config.h>
#include <octave/oct.h>

DEFUN_DLD (foo, args, , " ")
{
  NDArray a;

  a.print_info (octave_stdout, " ");
  return octave_value ();
}

which returns

octave:1> foo
 rep address: 0x8a4fee8
 rep->len:    0
 rep->data:   0
 rep->count:  2

Regards
David



--
David Bateman                                [hidden email]
Motorola CRM                                 +33 1 69 35 48 04 (Ph)
Parc Les Algorithmes, Commune de St Aubin    +33 1 69 35 77 01 (Fax)
91193 Gif-Sur-Yvette FRANCE

The information contained in this communication has been classified as:

[x] General Business Information
[ ] Motorola Internal Use Only
[ ] Motorola Confidential Proprietary


Reply | Threaded
Open this post in threaded view
|

Re: Memory Leak in Array<T> class

Paul Thomas-10
David,

I notice that you do not provide a solution.  Is there some subtlety that I
am missing or does replacing

>   Array (void)
>     : rep (nil_rep ()), dimensions (),
>       idx (0), idx_count (0) { rep->count++; }

with

>   Array (void)
>     : rep (nil_rep ()), dimensions (),
>       idx (0), idx_count (0) { }

not fix the problem?

Paul T


Reply | Threaded
Open this post in threaded view
|

Re: Memory Leak in Array<T> class

David Bateman-3
In reply to this post by David Bateman-3

> I notice that you do not provide a solution.  Is there some subtlety that I
> am missing or does replacing

I thought the solution was obvious... However, as you say, there is some
subtlety here that I haven't quite fathomed. Since when I tried you solution
on some related code I've been working on, the result was seg-faults in
completely unrelated parts of the code..

In any case if it is a memory leak, its a minor one, as the void contructors,
having no data, are quite small.

Cheers
David



>

--
David Bateman                                [hidden email]
Motorola CRM                                 +33 1 69 35 48 04 (Ph)
Parc Les Algorithmes, Commune de St Aubin    +33 1 69 35 77 01 (Fax)
91193 Gif-Sur-Yvette FRANCE

The information contained in this communication has been classified as:

[x] General Business Information
[ ] Motorola Internal Use Only
[ ] Motorola Confidential Proprietary


Reply | Threaded
Open this post in threaded view
|

Memory Leak in Array<T> class

John W. Eaton-6
In reply to this post by David Bateman-3
On 30-Oct-2004, David Bateman <[hidden email]> wrote:

| All of the void constructors derived from Array<T> have a memory leak. The
| bit of the class that describes the problem is
|
| template <class T>
| class
| Array
| {
| protected:
|   class ArrayRep
|   {
|   public:
|     ArrayRep (void) : data (0), len (0), count (1) { }
|   };
| private:
|
|   typename Array<T>::ArrayRep *nil_rep (void) const
|     {
|       static typename Array<T>::ArrayRep *nr
| = new typename Array<T>::ArrayRep ();
|
|       return nr;
|     }
|
|   Array (void)
|     : rep (nil_rep ()), dimensions (),
|       idx (0), idx_count (0) { rep->count++; }
| };
|
| So what happens is that the void constructor calls nil_rep which creates
| the internal representation and adds one to the count. A new ArrayRep
| is created for each void constructor. However the constructor itself
| then adds 1 to rep->count and the ArrayRep therefore becomes immortal
| as its count can never reach 1, when the Array<T>::~Array destructor
| is called. This can easily be demostrated with the code snippet.
|
| #include <octave/config.h>
| #include <octave/oct.h>
|
| DEFUN_DLD (foo, args, , " ")
| {
|   NDArray a;
|
|   a.print_info (octave_stdout, " ");
|   return octave_value ();
| }
|
| which returns
|
| octave:1> foo
|  rep address: 0x8a4fee8
|  rep->len:    0
|  rep->data:   0
|  rep->count:  2

If this code caused a memory leak, then running the above in a loop
like this:

  page_screen_output = 0;
  while (1)
    foo;
  endwhile

or, to speed things up a bit, modifying it to be

  #include <octave/config.h>
  #include <octave/oct.h>

  DEFUN_DLD (xxx, args, , " ")
  {
    NDArray a;
    return octave_value ();
  }

and running the same loop would cause Octave to grow without bound
(even if the growth was slow).  But that is not what I see.

The key is the definition of the nil_rep function:

  typename Array<T>::ArrayRep *nil_rep (void) const
    {
      static typename Array<T>::ArrayRep *nr
        = new typename Array<T>::ArrayRep ();

      return nr;
    }

Note that the returned value is static within the function, so it is
constructed just once, when NR is initialized.  Each time nil_rep is
called, we return the same empty ArrayRep object.  It is correct to
increment the reference count for this object in the constructors,
because we never want the static NR object to be destroyed.  Defining
nil_rep this way is an attempt to speed things up by avoiding
unnecessary calls to new and delete to repeatedly create an empty
ArrayRep object.  Since all empty ArrayRep objects have the same
representation, we should only have to create it once.

jwe


Reply | Threaded
Open this post in threaded view
|

Re: Memory Leak in Array<T> class

David Bateman-3
According to John W. Eaton <[hidden email]> (on 11/01/04):

> If this code caused a memory leak, then running the above in a loop
> like this:
>
>   page_screen_output = 0;
>   while (1)
>     foo;
>   endwhile
>
> or, to speed things up a bit, modifying it to be
>
>   #include <octave/config.h>
>   #include <octave/oct.h>
>
>   DEFUN_DLD (xxx, args, , " ")
>   {
>     NDArray a;
>     return octave_value ();
>   }
>
> and running the same loop would cause Octave to grow without bound
> (even if the growth was slow).  But that is not what I see.
>
> The key is the definition of the nil_rep function:
>
>   typename Array<T>::ArrayRep *nil_rep (void) const
>     {
>       static typename Array<T>::ArrayRep *nr
> = new typename Array<T>::ArrayRep ();
>
>       return nr;
>     }
>
> Note that the returned value is static within the function, so it is
> constructed just once, when NR is initialized.  Each time nil_rep is
> called, we return the same empty ArrayRep object.  It is correct to
> increment the reference count for this object in the constructors,
> because we never want the static NR object to be destroyed.  Defining
> nil_rep this way is an attempt to speed things up by avoiding
> unnecessary calls to new and delete to repeatedly create an empty
> ArrayRep object.  Since all empty ArrayRep objects have the same
> representation, we should only have to create it once.


Amazing what a difference a single "static" keyword makes in this
case. I'd missed that...

Cheers
David

--
David Bateman                                [hidden email]
Motorola CRM                                 +33 1 69 35 48 04 (Ph)
Parc Les Algorithmes, Commune de St Aubin    +33 1 69 35 77 01 (Fax)
91193 Gif-Sur-Yvette FRANCE

The information contained in this communication has been classified as:

[x] General Business Information
[ ] Motorola Internal Use Only
[ ] Motorola Confidential Proprietary