Simple matrix class - version 2

Question

As suggested on my previous question, I am posting my revised code for feedback.

One thing that caught my attention is that my program actually runs slower after adding the modifications. It was previously running near 128 iterations per second on a single core machine and 382 in a 4-core machine. It now runs at 125 in the single core and 360 in the 4-core machine. I am not sure the way I was managing memory in my first version is faster or if I implemented it wrong.

Grid3d.h:

#ifndef _GRID3D_
#define _GRID3D_

#include <iostream>

class Grid3d
{
private:
    size_t M, N, L;
    double* buffer;
    size_t compute_index(size_t, size_t, size_t) const;

public:
    //Constructores
    Grid3d(size_t,size_t,size_t);
    Grid3d();
    Grid3d(const Grid3d&);
    ~Grid3d();

    //Operadores
    double& operator()(size_t, size_t, size_t);
    double  operator()(size_t, size_t, size_t) const;
    Grid3d& operator=(Grid3d);

    //Acceso
    int get_L();
    int get_M();
    int get_N();
    double get(size_t,size_t,size_t) const;
    void reset();
};

std::ostream & operator<< (std::ostream &,Grid3d);
#endif

Grid3d.cc:

#include "Grid3d.h"
#include <cmath>
#include <iomanip>
#include <cassert>

using namespace std;

//------------------------------------------------------------------
// Constructores 
//------------------------------------------------------------------

//Constructor
Grid3d::Grid3d(size_t M, size_t N, size_t L)
      : M(M), N(N), L(L), buffer(new double[M * N * L])
{
    for (size_t l=0;l<M*N*L;l++){
        buffer[l] = NAN;
    }
}

//Constructor vacío
Grid3d::Grid3d()
    :M(0), N(0), L(0)
{
    buffer = NULL;
}


//Constructor copia
Grid3d::Grid3d(const Grid3d &other)
    :M(other.M), N(other.N), L(other.L)
{
    buffer = new double[M * N * L];
    for (size_t l=0;l<M*N*L;l++){
        buffer[l] = other.buffer[l];
    }
}

//Destructor
Grid3d::~Grid3d() { delete [] buffer; }

//------------------------------------------------------------------
// Operadores 
//------------------------------------------------------------------

//Access operator ():
double& Grid3d::operator()(size_t i, size_t j, size_t k)
{
    assert(i >= 0 and i < M);
    assert(j >= 0 and j < N);
    assert(k >= 0 and k < L);
    return buffer[compute_index(i, j, k)];
}

//Access operator () const:
double Grid3d::operator()(size_t i, size_t j, size_t k) const
{
    assert(i >= 0 and i < M);
    assert(j >= 0 and j < N);
    assert(k >= 0 and k < L);
    return buffer[compute_index(i, j, k)];
}

//Assignment operator
Grid3d& Grid3d::operator=(Grid3d other)
{
    using std::swap;
    swap(M, other.M);
    swap(N, other.N);
    swap(L, other.L);
    swap(buffer, other.buffer);
    return *this;
}

//------------------------------------------------------------------
// Acceso 
//------------------------------------------------------------------
size_t Grid3d::compute_index(size_t i, size_t j, size_t k) const
{
    return k * (M * N) + i * N + j;
}

int Grid3d::get_L()
{
    return L;
}

int Grid3d::get_M()
{
    return M;
}

int Grid3d::get_N()
{
    return N;
}

double Grid3d::get(size_t i, size_t j, size_t k) const
{
    assert(i >= 0 and i < M);
    assert(j >= 0 and j < N);
    assert(k >= 0 and k < L);
    return buffer[compute_index(i, j, k)];
}

void Grid3d::reset()
{
    for (size_t l=0;l<M*N*L;l++){
        buffer[l] = NAN;
    }
}

//------------------------------------------------------------------
// Others
//------------------------------------------------------------------

//cout
std::ostream & operator << (std::ostream & os , Grid3d g)
{
    int M = g.get_M();
    int N = g.get_N();
    int L = g.get_L();

    for (int k=0;k<L;k++){
        cout << "k = " << k << endl;
        for (int i=0;i<M;i++){
            for (int j=0;j<N;j++){
                cout.width(10);
                os << setprecision(3) << g.get(i,j,k)<< " ";
            }
            os << endl;
        }   
        os << endl;
    }
    return os;
}

Answer 1

I will expand on this when I have more time, but a few things jumped out:

---

get_M/get_L/get_N should return a std::size_t, not an int.

---

get_M/get_L/get_N should be const.

---

It might make sense in your field of study, but to me, NAN is weird for the initial value. I would expect the initial values to be either undefined or 0. Whatever works best for you though. Initializing to NAN would, after all, provide a few convenient properties.

---

All of the size_t in the header should be std::size_t.

---

Rather than duplicate the exact same code, the constructor should use reset.

---

Be careful with using namespace std;, even in implementation files. If you want to read way more than you ever wanted to on it, see this. The "tl;dr" of it is that a potentially application breaking bug might happen years down the road do to a change a dozen files away, but that can all be avoided with 5 extra characters (std::).

---

When outputting potentially huge chunks of data, try not to flush until you either need to or the end (in single threaded applications, that's almost always going to be the end).

In particular, std::endl is not equivalent to "\n". std::cout << "\n"; pushes a new line into std::couts buffer. std::cout << std::endl; pushes a newline into std::cout's buffer and then flushes the buffer. This means that your operator<< isn't really using buffered IO since it flushes after every output.

Simple fix: use "\n" and then just do std::flush(std::cout); (or std::cout << std::flush) at the end.

---

cout.width(10); in your operator<< should be os.width(10).

Answer 2

Most likely the performance degradation is caused because in your previous version, operator<< took a Grid3d&, and in this version it takes a Grid3d. Your operator<< signature should be

std::ostream& operator<<(std::ostream&, const Grid3d&);

As another code improvement, you should change your copy constructor:

Grid3d::Grid3d(const Grid3d &other)
  : L(other.L), M(other.M), N(other.N)
{
    buffer = new double[L * M * N];
    std::memcpy(buffer, other.buffer, M * N * L * sizeof(double));
}

memcpy may give you better performance than the for loop you wrote.

Answer 3

So you do not have to handle NULL created a zero sized array here (and use the initializer list):

//Constructor vacío
Grid3d::Grid3d()
    :M(0), N(0), L(0)
{
    buffer = NULL;
}

// Prefer
Grid3d::Grid3d()
    :M(0), N(0), L(0), buffer(new double[N*M*L])
{}

By creating the array like this, the rest of you code does not need to have a special case for NULL buffer. The code just looks the same.

Now that looks very similar to your other constructor.
You could potentially just combine the two.

Prefer to use the initializer liest (even in the copy constructor)

Grid3d::Grid3d(const Grid3d &other)
    :M(other.M), N(other.N), L(other.L)
    ,buffer(new double[M * N * L])
{
    for (size_t l=0;l<M*N*L;l++){
        buffer[l] = other.buffer[l];
    }
}

For the const version of your access operator. Just make it call the non const version. (We know that neither version actually mutate the object so there is no harm in casting away constness). The const version returns a value so it is not exposing any data for modification:

double Grid3d::operator()(size_t i, size_t j, size_t k) const
{
    return const_cast<Grid3d>(*this)(i,j,k);
}

All the following should be const:

int Grid3d::get_L()
int Grid3d::get_M()
int Grid3d::get_N()

Pass by const reference when you can to prevent an expensive copy.

std::ostream & operator << (std::ostream & os , Grid3d const& g)
                                               //      ^^^^^^

Prefer not to use \n instead of std::endl. It has no major benefits and can slow code down dramatically. If you want to guarantee a flush print the whole structure first then use std::endl instad of the last '\n'.