1

I maintain a old C++ application which has a lot of classes like below:

    class ClassWithALotOfVectors
    {
        std::vector<int> _vector1;
        std::vector<int> _vector2;

        // a lot of other vector datamembers go here

        std::vector<double> _vectorN;
    };

that is - a data type where members a vectors of double or int. The thing is - these vectors are never populated at the same time - and therefore when we create 100000 instances of ClassWithALotOfVectors - memory usage adds up to impressive number even though only 10% of these vector are in use.

So I decided to write a small "allocate on demand" vector class.

It is a wrapper around std::vector - where internal vector only create when accessed for the first time (using two getters - ref() & const_ref() methods)

When I replaced std::vector in a ClassWithALotOfVectors class with a compact_vector as below:

    class ClassWithALotOfVectors2
    {
    compact_vector<int> _vector1;
    compact_vector<int> _vector2;

    compact_vector<double> _vectorN;
    };

did a few tests and the results were promising - memory usage went down dramatically, but suddenly found that application does not release memory at the end - the memory consumptions grows in much slower rate than it is used to be - but application does not seem to be deallocating the memory at the end.

Can you look at my implementation of the compact_vector and see if you can spot something wrong with a memory management.

    template <class T> class compact_vector 
    { 
    public: 
            compact_vector<T>()
            :_data(NULL) 
            { 
            } 

            ~compact_vector<T>() 
            { 
                    clear(); 
            } 

            compact_vector<T>(const compact_vector<T> & rhs) 
            :_data(NULL) 
            { 
                    if (NULL != rhs._data) 
                    { 
                            _data = new std::vector<T>(*(rhs._data)); 
                    } 
                    else 
                    { 
                            clear(); 
                    } 
            } 

            //      assignment 
            // 
            compact_vector<T> & operator=(const compact_vector<T> & rhs) 
            { 
                    if (this != &rhs) 
                    { 
                            clear(); 
                            if (NULL != rhs._data) 
                            { 
                                    _data = new std::vector<T>(*rhs._data); 
                            } 
                    } 
                    return *this; 
            } 
            compact_vector<T> & operator=(const std::vector<T> & rhs) 
            { 
                    clear(); 
                    if (!rhs.empty()) 
                    { 
                            _data = new std::vector<T>(rhs); 
                    } 
                    return *this; 
            } 

            const std::vector<T> & const_ref() 
            { 
                    createInternal(); 
                    return *_data; 
            } 
            std::vector<T> & ref() 
            { 
                    createInternal(); 
                    return *_data; 
            } 
            void    clear() 
            { 
                    if (NULL != _data) 
                    { 
                            _data->clear(); 
                            delete _data; 
                            _data = NULL; 
                    } 
            } 
    private: 
            void    createInternal() 
            { 
                    if (NULL == _data) 
                    { 
                            _data = new std::vector<T>(); 
                    } 
            } 

    private: 
            compact_vector<T>(const std::vector<T> & rhs) 
            { 
            } 
            compact_vector<T>(std::vector<T> & rhs) 
            { 
            } 
            compact_vector<T>(std::vector<T> rhs) 
            { 
            } 

            std::vector<T> * _data; 
    }; 
Andrew
  • 291
  • 1
  • 10
  • 3
    Your `clear` method can be dramatically simplified to `delete _data; _data = 0;` – Konrad Rudolph Mar 19 '12 at 11:16
  • 1
    You should use `nullptr` nowadays if possible, though `NULL` is still ok. – leftaroundabout Mar 19 '12 at 11:19
  • 1
    There is a [code review](http://codereview.stackexchange.com/) section of [stack exchange](http://stackexchange.com/) – Peter Wood Mar 19 '12 at 11:28
  • @Peter As their FAQ states, [codereview.se] is for working code. This question is actually about a problem in the code: there's a memory leak. I think it's appropriate here. – R. Martinho Fernandes Mar 19 '12 at 11:30
  • @R.MartinhoFernandes Yes, I see that now. It wasn't clear to me quickly scanning through that there was a problem. Maybe edit the question to make it more obvious what help is being asked for. – Peter Wood Mar 19 '12 at 11:35
  • In many cases, `std::deque` can be used as a stand-in for `std::vector`. Because it doesn't use contiguous storage the memory allocations are smaller and easier recycled. – MSalters Mar 19 '12 at 12:01
  • @MSalters: While technically you're right, having a `compact_vector` which is silently not contigious in memory might be confusing to users (many of which will assume that it's a vector, hence contigious). – ev-br Mar 19 '12 at 12:20

4 Answers4

3

Most implementations of std::vector don't acquire memory until you need it, and the size of a vector is usually just a few (3 + possibly extra debug information) pointers. That is, std::vector<int>() will not allocate space for any object. I believe that you are barking at the wrong tree here.

Why is your original code having a much higher memory usage? Are you expecting clear() to release memory?

If so you are mistaken. The clear() function destroys the contained elements but does not release the allocated memory. Consider adding a wrapper to clear the memory that uses the following idiom:

std::vector<int>().swap( _data );

What the previous line does is creating a new empty vector (usually no memory attached, not mandated, but common implementation) and swapping the contents of the two vectors. At the end of the expression, the temporary contains the data that was originally held by the _data vector and _data is empty and with no memory (implementation defined). The temporary is destructed at the end of the full expression and memory is released.

Alternatively, if your compiler supports C++11, you can use shrink_to_fit() after calling clear(). The standard does not require shrink_to_fit() to actually shrink to fit, but for an empty vector I would expect the implementation to do it. Test it by calling capacity() after the call and seeing whether it has gone down to 0 or not.

David Rodríguez - dribeas
  • 192,922
  • 20
  • 275
  • 473
  • even if clear() would not release the memory, the next line delete would do, wouldn't it? – Andrew Mar 19 '12 at 12:30
  • @Andrew I am talking about the original piece of code, not the `compact_vector`. While you can fix `compact_vector`, that is just adding complexity that I believe not to be necessary. Yes, the destruction of the vector will release the memory, just as the swap with the temporary, but the latter removes a good amount of code from your application and thus reduces the possibility of errors, including the current memory leak. – David Rodríguez - dribeas Mar 19 '12 at 12:32
  • 1
    @Andrew: In case it is not clear: I propose removing the bugs in `compact_vector` by removing the need for the whole type. If your memory footprint is high just due to the *empty* vectors, then you might want to actually consider the design. – David Rodríguez - dribeas Mar 19 '12 at 12:40
  • 1
    the original code has had a high memory usage because each ClassWithALotOfVectors class has 50 vectors, where only 10 of these 50 were actually used. You say that the empty std::vector only has a memory overhead of 3-5 pointers, while the memory overhead in my compact_vector is only a single pointer (_data), which makes it 3-4 times more memory efficient in most of the my use cases (as I said only 10 out of 50 vectors are non-empty) – Andrew Mar 19 '12 at 12:45
  • 1
    The minimum possible size of a fully functioning `std::vector` is one pointer size and two integer sizes (12 byte on 32bit). Most implementations tend to use 3 pointers though, since the sizes are the same and it's usually a bit easier / cleaner that way. Still, 12 byte on 32bit. And then you're ignoring something important.. the overhead of actually getting your `_data` on the heap. Any trip to the heap is expensive, and should best be avoided. Screw a few extra byte on the stack (or are those `compact_vector`s expected to be on the heap? I hope not). – Xeo Mar 19 '12 at 12:50
  • 1
    @David - I see your point about design here and I would agree, if it were not a **old** app and I would rather not to change much, hence my intention with a compact_vector is to provide a simple/non-intrusive fix to memory issues. – Andrew Mar 19 '12 at 12:51
  • @Xeo, what do you mean "then you're ignoring something important.. the overhead of actually getting your _data on the heap" - what sort of "overhead" do you refer here? And yes - compact_vector are used as datamembers in classes which are created on the heap, do you see any issues with that? Thanks. – Andrew Mar 19 '12 at 13:06
1

Use smart pointers. In this case, std::unique_ptr with hand-made copy ctor / assignment operator. Suddenly, all your problems are solved. It's like magic!

Sorry to sound so snarky, but memory leaks always have the same answer: smart pointer.

Community
  • 1
  • 1
Xeo
  • 123,374
  • 44
  • 277
  • 381
  • Wouldn't that increase the memory consumption to about what the original vector had? – Bo Persson Mar 19 '12 at 11:47
  • @BoPersson Any sane implementation of `unique_ptr` has the same footprint as a pointer. – R. Martinho Fernandes Mar 19 '12 at 12:07
  • 1
    @Xeo unfortunately I can't use std::unique_ptr as it is only available in the latest version of C++ while my application has to be compiled on Visual Studio 2005, which is 5 years away from being C++0x compliant. More over - I can't really see where my compact_vector is leaking the memory if at all... – Andrew Mar 19 '12 at 12:18
  • 1
    @Andrew: Boost has `unique_ptr` and `scope_ptr`. If your object does not need to be *copyable* or used in a container, then you can also use *good old* `std::auto_ptr`. – David Rodríguez - dribeas Mar 19 '12 at 12:27
  • @DavidRodríguez-dribeas Not sure of Andrew's case but many companies stick to the libraries they already use. – Jagannath Mar 20 '12 at 09:27
0

The idea that only one vector at a time is ever in use sounds like the union concept. Sure enough, C++ has an element called an anonymous union that does precisely this.

class ClassWithALotOfVectors {
    public:
    union  {
        std::vector<double> _vector1;
        std::vector<double> _vector2;
    };
    ClassWithALotOfVectors() { new(&_vector1) std::vector<double>; };
    ~ClassWithALotOfVectors() { _vector1.~vector<double>(); };
};

Because the union can't know which element's constructor to call, default constructors are disabled in the union and you have to manually construct and deconstruct the union's element, but I think this will accomplish what you are looking for, aliasing _vector1 and _vector2 to the same element but placing both in the ClassWithALotOfVectors namespace, then deallocating the vector when the destructor for ClassWithALotOfVectors is called.

For more on anonymous unions see: CPP reference: Anonymous Unions

mayo
  • 3,314
  • 1
  • 32
  • 40
bgulko
  • 1
  • 1
0

Since you are saying it's an old application and can't do much with it. How about having all the vectors having zero size reserved during construction.
It's tedious, but since old app and not much is expected to be modified. May be worth the effort for once.

class ClassWithALotOfVectors    
{    
   std::vector<int> _vector1;     
   std::vector<int> _vector2;        
   // a lot of other vector datamembers go here         
   std::vector<double> _vectorN;

  public:
     ClassWithALotOfVectors() : Init() { }   
     void Init()   
     {   
         vector1.reserve(0);   
         /// Like wise other vectors.   
     }  
};
Jagannath
  • 3,918
  • 22
  • 28