3

Here is my code in c++

int** a;
try{
  a = new int*[m];
  for(int i = 0; i<m;i++)
    a[i] = new int[n];
}

... Right now i am initializing the above using for loops as follows:

for(int i = 0; i<m; i++)
  for(int j = 0; i<n; j++)
      a[i][j] = 0;

I am trying to improve performance and hence thought using memset would be a good idea . So modified my code to use memset instead of for loop as follows:

memset(a, 0, sizeof(a[0][0]) * m * n);

But i get Segmentation fault on executing this . Can anybody help me figure out what I am doing wrong?

Improve this question
3
  • Do you allocate memory for "a" anywhere in your code? Commented Mar 22, 2013 at 18:59
  • Sorry, I missed to add memory allocation. a = new int*[m]; for(int i =0; i<m ;i++) a[i] = new int[n]; Commented Mar 22, 2013 at 19:03
  • Ok, now the problem is clear - added an answer. Commented Mar 22, 2013 at 19:08

5 Answers 5

Reset to default
4 
int** a;

This just gives you a single object. A int** object. It doesn't point anywhere at all. There are no ints to assign to. When you start assigning to the ints as though they exist, you get undefined behaviour.

In addition, the memory layout of an int** pointing to a "2d array" of ints is like so: the int** points at the first element in an array of int*s, and the int*s point at the first elements in an array of ints. This memory is not contiguous because it requires indirection to jump around memory, i.e. it's not a single block of memory. You can't just write to it using memset.

If you just want a fixed compile-time sized 2D array of ints, do:

int a[N][M];

where N and M are constant expressions. This is stored contiguously, but I still don't recommend using memset.

Or use a standard container, such as:

std::array<std::array<int, M>, N> a;

If you need it of dynamic size, try:

std::vector<std::vector<int>> a(M, std::vector<int>(N));

Alternatively, you can stick with your int** and make sure you dynamically allocate the int*s and ints:

int** a = new int*[M];
for (i = 0; i < N; i++) {
  a[i] = new int[N];
}

But this is ugly!

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

int a[N][M]; - Yet part of C99 and available also as an extension of GCC, variable length arrays are not part of C++ standard.
@LihO Where N and M are constant expressions.
ya right now i m using the ugly part u just described. So will improve it
3 
int** a;

is just a declaration of a pointer to pointer to int.

"Right now i am initializing the above using for loops"

You are not initializing it in your for loops, you're just trying to assign 0 to the elements that don't exist, which produces an undefined behavior. You need to either dynamically allocate the memory for these elements or yet even better: use std::vector instead:

std::vector< std::vector<int> > a(m, std::vector<int>(n, 0));

"I am trying to improve performance"

Don't do that unless it is necessary. Don't optimize prematurely.

EDIT: After you mentioned that you are facing the performance issues already, here's what you could do: Instead of this two-dimensional C-style array:

int** a = new int*[m];      // m = number of rows
for(int i = 0; i < m; i++)
    a[i] = new int[n];      // n = number of columns

you could use one-dimensional std::vector:

std::vector<int> vec(rows * cols, 0);
...
vec[i * cols + j] = 7;   // equivalent of vec[i][j]
this will have more advantages:
  • your 2D array will be stored within the continuous block of memory
  • this block of memory will be allocated at once, not in many small pieces
  • frequent accessing of elements will be faster thanks to spatial locality
    (elements that are "near" will be available in cache memory thus your
    program will not have to load them from the main memory)
  • and you will not be responsible for the memory management
    (memory will be cleaned up automatically once the vector object is destructed)
Improve this answer

4 Comments

Main reason i m trying to optimize it is using for loops is taking long time and i have to cut it down
Are you sure that for loops take a lot of time? Did you compile with a recent compiler (e.g g++ from GCC 4.7 or 4.8) and optimizations enabled (e.g. -O2 or -O3)?
I now see the difference. I compiled using g++ and optimization enabled and its faster. But the actual code which i was trying to optimize does use these option. Thanks
@user2175966: See my answer now, the part under the line :)
1

With an int ** you won't normally have a single, contiguous block of memory. Assuming you use it correctly, you'll have an array of pointers. Each of those pointers will then have an array allocated for it individually.

That being the case, you can't convert your loops to a single memset (and still get defined behavior).

Improve this answer

Comments

0

I think problem is with the memory not allocated for actual storage. Variable a is only pointer (in addition not initialized). To what place it points?

Improve this answer

Comments

0

You said you allocated it like this:

a = new int*[m]; 
for(int i =0; i<m ;i++) a[i] = new int[n];

Like Jerry Conffin said - This won't give you a single, contiguous block of memory. Each new array (new int[n]) will be allocated in a possibly totally different location and memset only works on contiguous blocks, so you have to reset each of them "by hand" Btw - I'm pretty sure you're not going to see any performance improvements from using memset over a loop (memset itself use implemented using a loop I think).

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.