0

I have a function that returns a vector of a class:

vector<movement> returnMoves(int startx, int starty, int bb[][8], int side){

vector<movement> moves;
movement adding;
moves.push_back(adding); moves.push_back(adding); //example

return moves;
}

And I am calling the function in this way from the main:

vector<movement> t1;
t1 = returnMoves(startx, starty, bb, 1);

It works, but this process is being done many many times, and it's slow, so I'd like to make it faster so I was considering returning by reference or by pointers: This is what I tried:

vector<movement> & returnMoves(int startx, int starty, int bb[][8], int side){
vector<movement> temp1;
vector<movement>& moves = temp1;
moves.push_back(adding); moves.push_back(adding);
return moves;
}

and calling it in the same way:

 t1 = returnMoves(startx, starty, bb, 1);

It gives me a segmentation fault, what am I doing wrong?

Ori
  • 23
  • 5
  • you are referencing undefined pointers. – Irrational Person Dec 28 '14 at 01:14
  • 1
    A `std::vector` is already just a reference to the actual memory containing the objects, so your original code is just copying the references (if NRVO isn't applied to construct the vector directly where the caller needs it). This is the benefit of C++11 "move constructor / move assignment". If you are seeing a slowdown from the original, correct code, get a better compiler. – Ben Voigt Dec 28 '14 at 01:28
  • `It works, but this process is being done many many times, and it's slow,` You have proof that it's slow? Or are you guessing it will be slow? Also, if you did measure, did you measure an optimized build of the program? – PaulMcKenzie Dec 28 '14 at 01:29
  • Do note that NRVO isn't possible in your case because the temp vector isn't created during initialization. Can you replace assignment by initialization? – Ben Voigt Dec 28 '14 at 01:31
  • Finally, if you don't have and can't get a compiler with C++11 rvalue reference support, use `swap`. – Ben Voigt Dec 28 '14 at 01:34
  • @BenVoigt thanks for explaining about the C++11 "move constructor/move assignment", in fact, it seems like even when I passed by reference the speed didn't improve, (maybe 5-10%, but I might be humanly wrong). Pardon my basic knowledge, but what did you mean when you asked if I can replace assignment by initialization? – Ori Dec 28 '14 at 02:13
  • You have `vector t1; t1 = ....` which is assignment. For initialization, use `vector t1 = ....` – Ben Voigt Dec 28 '14 at 02:25
  • Following Pradhan's suggestion, now my program is slightly faster, so my function is vector legalmoves; returnMoves(legalmoves, t2, t1, bb, 1); So, in this case, why would I want to write vector legalmoves = .. ? would initializing it before make it faster? – Ori Dec 28 '14 at 02:31
  • @Ori: Faster than your original code yes, faster than the reference argument no. The reason the reference argument is winning is because you're reusing the same `vector` object which already has a big enough buffer, so there isn't repeated reallocation. – Ben Voigt Dec 28 '14 at 02:48

1 Answers1

3

You are returning a reference to a local variable. The variable temp1 goes out of scope after returnMoves exits, leading to undefined behaviour. If you want to avoid copies, you could simply pass in the object that you want filled like this:

void generateReturnMoves(vector<movement>& populateThis, int startx, int starty, int bb[][8], int side){
//Add all the movement objects you need to populateThis
}

Now, you can use it as:

vector<movement> t1;
generateReturnMoves(t1, startx, starty, bb, 1);
Pradhan
  • 16,391
  • 3
  • 44
  • 59
  • 1
    You can instead return a pointer, but you'll have to allocate the vector with new. Either allocate in the function or have the client pass in an allocated pointer. Just make sure it's clear that whoever uses the function is in charge of calling delete when they're done. – Dtor Dec 28 '14 at 01:16
  • thanks Pradhan, any chance you could tell me exactly what to change? I am not familiar with pointers yet – Ori Dec 28 '14 at 01:17
  • @Ori Edited it into the answer. – Pradhan Dec 28 '14 at 01:17
  • @CaptainObvlious Returning by value causes it to be created twice, once in the function and again when returned. Returning with pointer only creates the vector once then creates just a pointer for the return. In most cases, the edited answer with it being passed in and returned by reference is probably preferable. – Dtor Dec 28 '14 at 01:29
  • @Dtor: Not in modern C++ it doesn't. Returning a local variable is one of the places where move kicks in automatically. – Ben Voigt Dec 28 '14 at 01:29
  • @Dtor - `Returning by value causes it to be created twice, once in the function and again when returned.` No. You cannot predict how many times the copy constructor will be called. There is (N)RVO optimization as well as the C++ 11 move constructors that elide copy creation. – PaulMcKenzie Dec 28 '14 at 01:36
  • @Pradhan I tried this solution, it makes it more than 3 times slower, how can that be? – Ori Dec 28 '14 at 01:40
  • @PaulMcKenzie: While you can't predict how many times the move constructor will be called, you're guaranteed the copy constructor is never called. – Ben Voigt Dec 28 '14 at 01:40
  • @Ori LOL, probably compiler was able to do optimizations that can not be done when function operates on referenced arguments. Return by value is not as slow as people tend to think. – mip Dec 28 '14 at 01:42
  • It's for a chess program that I work on as a hobby, if anyone knows a simple way to optimize that code to make if faster, it would be awesome – Ori Dec 28 '14 at 01:45
  • @Ori It might be better to open a new question with showing the two implementations and their runtimes. But a quick question - are you clearing `t1` between consecutive calls to `returnMoves`? If not, you would just be growing it along with the values from the earlier runs. That would be a big slowdown because of `vector`'s capacity changes. And related to that topic would be usage of `vector::reserve`. – Pradhan Dec 28 '14 at 01:46
  • @Ori chess is not very dynamic game, so if you use same vector many times just generate it once per turn and store somewhere. – mip Dec 28 '14 at 01:48
  • @Pradhan your intuition was right! I was not clearing t1, now that I do, it's fast again, but not much faster than the original version, maybe 5-10% faster (human estimation lol) – Ori Dec 28 '14 at 01:55