C++ simple matrix class, improve coding 2

Question

as suggested on my previous question, C++ simple matrix class, improve coding, I am posting my revised code for feedback. I followed ruds's suggestion about memory management instead of using std::vector. Thanks also to Lstor and Corbin for their help too.

One thing that called my atention is that my program actually runs slower after adding the modifications, before it was running near 128 iterations per second on a single core machine and 382 in a 4-core machine, now it 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 I implemented it wrong.

Here is the code:

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;
}

EDIT: Here is an updated version including your suggestions

Grid3d.h:

#ifndef _GRID3D_
#define _GRID3D_

#include <iostream>

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

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

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

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

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

Grid3d.cc:

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

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])
{
    reset();
}

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


//Copy constructor
Grid3d::Grid3d(const Grid3d &other)
    :M(other.M), N(other.N), L(other.L)
{
    double* tmp = new double[M * N * L];
    //delete [] buffer; <-- this line gives me a "core generated" error
    buffer = tmp;
    std::memcpy(buffer, other.buffer, M * N * L * sizeof(double));
}

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

//------------------------------------------------------------------
// Operators
//------------------------------------------------------------------

//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;
}

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

size_t Grid3d::get_L() const
{
    return L;
}

size_t Grid3d::get_M() const
{
    return M;
}

size_t Grid3d::get_N() const
{
    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 const & g)
{
    size_t M = g.get_M();
    size_t N = g.get_N();
    size_t L = g.get_L();

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

Are you compiling with optimization on? This is probably important, as with optimization off, compute_index will not be inlined (for example). — ruds, Jun 25 '13 at 1:13
As another note, using NANs may slow down your code considerably. Floating point operations with NaNs are slower than those without. — ruds, Jun 25 '13 at 1:16
I won't post this as an answer since it's been covered in detail on your previous post, but I'd like to again strongly urge you to use a single dimensional vector. With optimizations, it will be as fast as a raw array. It will provide much cleaner and smaller code at literally 0 cost. I consider a bare array the wrong choice here unless performance actually does be one a concern and a bare array is faster (and it wouldn't be). — Corbin, Jun 25 '13 at 3:38
I would strongly recommend you to use English in your comments as well. Not only is it good practice, but it also helps in case you ever need to, say, post your code to an English-language online Q&A or code review site. — Lstor, Jun 25 '13 at 5:38
@ruds I am compiling using gcc 4.7 with -O2. I initialize the grid points as NAN, then i change each point to its propper value(which is generally not 0). That way, if I forget to define the value of a grid point I notice imediatley, so i am not doing float operation with nans. — user1995432, Jun 26 '13 at 3:00

Corbin · Answer 1 · 2013-06-25 05:31:25Z

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).

+1; instead of creating a separate answer, I'll just add a comment: in addition, I'd also replace all uses of int with std::size_t in operator << (which should also preferably take Grid3d by const-ref, 4 x 64-bit + alignment may be a bit too much for pass-by-value). Also, regardless of whether the OP adopts std::vector, I'd probably also use std::copy in the copy constructor. — Matt, Jun 25 '13 at 22:25
@Matt ruds already pointed out that operator<< should accept a const-ref, so I left that out. The size_t is a good point though. I had already noticed all the other size_t stuff by then, so wasn't still on the lookout for bad types :). std::copy is a good point too. I plan to modify my answer later though with a huge rant as to why double* is completely the wrong choice. Once it's a vector (hopefully), a copy constructor won't even be needed. std::copy is still a good point though. You should write that comment as an answer. C++ idiomatic-ness points are always good :) — Corbin, Jun 25 '13 at 22:35

ruds · Answer 2 · 2013-06-26 04:39:44Z

up vote 3 down vote

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.

edited Jun 26 '13 at 4:39

answered Jun 25 '13 at 1:22

ruds
1,618114

2

A small note, but the order of delete and new in your copy constructor should be the other way around. This ensures strong exception safety in the case that new throws. – Yuushi Jun 25 '13 at 2:41

1

Well, it can't be quite in the opposite order, but I'll edit the code to take your comment into account. – ruds Jun 25 '13 at 5:05

1

Will M, N and L of the object be left containing the wrong values if new throws? – William Morris Jun 25 '13 at 18:16

2

Err... on further thought, this is a constructor, so if new throws, the object get cleaned up and no longer exists, so the state of L, M and N is irrelevant. Sorry to confuse :-( – William Morris Jun 25 '13 at 20:17

BANG you are dead the shuttle just blew up. You are deleting an initialized member Buffer. It is not leaked . This is a CONSTRUCTOR so it has not previously been initialized. – Loki Astari Jun 25 '13 at 23:39

show 5 more comments

Loki Astari · Answer 3 · 2013-06-25 23:54:13Z

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'.

asked	9 months ago
viewed	181 times
active	9 months ago

current community

your communities

more stack exchange communities

C++ simple matrix class, improve coding 2

3 Answers

Your Answer

Not the answer you're looking for? Browse other questions tagged c++ or ask your own question.

Linked

Hot Network Questions

current community

your communities

more stack exchange communities

C++ simple matrix class, improve coding 2

3 Answers

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c++ or ask your own question.

Linked

Related

Hot Network Questions