Jesse_mw
Jesse_mw

Reputation: 115

Why is adding smart pointers to vector causing memory leak?

I am trying to generate objects to add to a vector for use in populating a database, when doing so I noticed a memory leak, I am sure it's something to do with the way I am adding the objects to the Vector due to the fact if I do not add any objects to the vector the function works as expected with no leaks. Where have I gone wrong?

I have tried moving the unique_ptr, same result.

I have found with this code, not all memory will be de-allocated each run of database::populate, after many executions of the populate function almost 400 mb of memory will not be de-allocated, why?

main.cpp

#include <iostream>
#include "database.h"
int main()
{
    std::vector<string> cols{"name", "score" };
    while (true) {
        std::getchar();
        database::populate("test", cols, 1000000);
 }

}

database.h

#include <string>
#include <vector>
using std::string;
class database {

public:

    static void  populate(const string table, std::vector<string> cols, int limit);
};

database.cpp

#include "database.h"
#include "model.h"

#include <memory>
#include <random>
#include <iostream>

typedef std::vector<std::shared_ptr<Model>> modelContainer;

static const char alphanum[] =
"0123456789"
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
"abcdefghijklmnopqrstuvwxyz";
//temporary
int alphaLen = std::size(alphanum) - 1;
std::random_device rd;
std::mt19937 gen(rd());
int random(int min, int max) {
    std::uniform_int_distribution<>rng_dice(min, max); return rng_dice(gen);
}
string randstring(int len) {
    string  str = "";

    while (len--) {
        int index = random(0, alphaLen);
        str += alphanum[index];
    }
    return str;
}
void database::populate(const string table, std::vector<string> cols, int limit) {
    modelContainer models;
    std::shared_ptr<Model> e;
    for (int i = 0; i < limit; i++) {
        e = std::make_shared<Model>(table);
        for (int j = 0; j < cols.size(); j++) {
            e->addVal(cols[j], randstring(5));
        }
        models.push_back(std::move(e)); //Leak??

    }
    std::cout << "done!" << std::endl;

    // add to database
}

model.h

#include <map>
#include<string>
#include <vector>
using std::string;
class Model {
private:
    std::map<string, string> data;
    string table;
public:
    Model(const string table);
    void addVal(const string& key, const string& val);
};

model.cpp

#include "model.h"
#include <string>
Model::Model(const string table) {
    this->table = table;


}
void Model::addVal(const string& key, const string& val) {
    (data)[key] = val;
}

Upvotes: 0

Views: 470

Answers (1)

Ted Lyngmo
Ted Lyngmo

Reputation: 117433

Your OS won't report exactly what you have allocated/freed when looking in the TaskManager or top, so I wouldn't start worrying about memory consumption in that program unless it increases over time. GNU has an extension in the form of function called malloc_stats() that you can call to report malloc/free statistics from within a program.

I scanned your program (using scan-build, address sanitizer, ubsan, ...) but couldn't find any problems and the memory consumption is holding at the same level when running in an endless loop. I used malloc_stats for checking the latter.


Some notes on the code since I read through it anyway:

  • Use header guards: #ifndef HEADER_H ... or #pragma once
  • Don't do using ... into the global namespace in header files. A user of such a header file could get surprises.
  • Pass parameters that are expensive to copy by const& instead of by value. There's currently some unnecessary string and vector copying.
  • Initialize class member variables in the constructors member initializer list. E.g.
    Model::Model(const string& Table) : data{}, table(Table) { /* now empty */ }
  • Use the correct type when using the subscript operator. You've used a signed integer (int), which triggers warnings, where an unsigned integer (size_t) would have been more appropriate.
  • Use reserve() on containers supporting it when you know how many entries you'll add.
  • Prefer unique_ptr over shared_ptr if you don't need sharing.
  • Prefer letting a standard container store the actual objects over storing unique_ptrs to the objects.
  • A uniform_int_distribution is very lightweight, but if you only need the same distribution all the time, you might as well reuse it.
  • std::mt19937 isn't thread-safe so if you want to use it from more than one thread, make it thread_local. This isn't a problem in your current code, but could be good to know.

database.cpp example

#include "database.h"
#include "model.h"

#include <iostream>
#include <memory>
#include <random>
#include <string_view>

using namespace std::literals::string_view_literals;
using std::string;
using modelContainer = std::vector<Model>; // no smart pointer needed

// A function to return a thread_local generator that is initialized once.
inline std::mt19937& generator() {
    // The std::random_device is created, called and disposed of since
    // we only need it to seed the std::mt19937 PRNG.
    static thread_local std::mt19937 gen(std::random_device{}());
    return gen;
}

// Made random into a function template to make it able to return the
// correct type and taking [min, max] as template parameters to only
// instantiate the distribution once per IntType and range used.
template<typename IntType, IntType min, IntType max>
inline IntType random() {
    static std::uniform_int_distribution<IntType> rng_dice(min, max);
    return rng_dice(generator());
}

string randstring(size_t len) {
    static constexpr std::string_view alphanum =
        "0123456789"
        "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
        "abcdefghijklmnopqrstuvwxyz"sv; // sv = string_view literal

    string str;
    str.reserve(len);
    while(len--) {
        auto index = random<size_t, 0, alphanum.size() - 1>();
        str += alphanum[index];
    }
    return str;
}

void database::populate(const string& table, const std::vector<string>& cols,
                        size_t limit) {
    modelContainer models;
    models.reserve(limit);
    for(size_t i = 0; i < limit; ++i) {
        Model e(table);
        for(size_t j = 0; j < cols.size(); ++j) {
            e.addVal(cols[j], randstring(5));
        }
        models.emplace_back(std::move(e));
    }
    std::cout << "done!" << std::endl;
}

Upvotes: 1

Related Questions