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;
}
  • 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
  • 2
    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
  • 4
    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
  • 1
    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

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
    +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

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.

  • 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. – Martin York Jun 25 '13 at 23:39

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

Your Answer

 
discard

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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