C++ Static inside function

Question

Hello i have a function that return an std::pair and is called very often.

std::pair<sf::Vector2i, sf::Vector2i> 
Map::map_coord_to_chunk_coord(int x, int y) {
  // Get the chunk position
  int chunk_x = x / CHUNK_SIZE;
  int chunk_y = y / CHUNK_SIZE;

  // Get the position inside the chunk
  x = x - chunk_x * CHUNK_SIZE;
  y = y - chunk_y * CHUNK_SIZE;

  // Return the chunk position and the position inside it
      return std::pair<sf::Vector2i, sf::Vector2i>(sf::Vector2i(chunk_x, 
chunk_y), sf::Vector2i(x, y));
}

Is this better to declare the pair as static so that it isn't created each time.

std::pair<sf::Vector2i, sf::Vector2i> 
Map::map_coord_to_chunk_coord(int x, int y) {
  static std::pair<sf::Vector2i, sf::Vector2i> coords;

  // Get the chunk position
  coords.first.x = x / CHUNK_SIZE;
  coords.first.y = y / CHUNK_SIZE;

  // Get the position inside the chunk
  coords.second.x = x - coords.first.x * CHUNK_SIZE;
  coords.second.y = y - coords.first.y * CHUNK_SIZE;

  // Return the chunk position and the position inside it
  return coords;
}

I run callgrind and it looks like this function is 3 time faster but is this a good practice ?

this coords will only be use inside this function? I mean you declare the object coords and only inside map_coord_to_chunk_coord get used of it? — marquesm91, May 15 '17 at 11:49
How did you measure the speed increase? I would think `return {{(chunk_x, chunk_y}, {x, y}};` would work just fine — NathanOliver, May 15 '17 at 11:50
Yes only this function use it, and by good practice i mean can this create some problem ? — thoregan, May 15 '17 at 11:51
`x - chunk_x * CHUNK_SIZE` == `x - (x / CHUNK_SIZE) * CHUNK_SIZE` - so this is basically `x = x - x` which equals zero (although integer division might do something)? — Felix Glas, May 15 '17 at 11:52
I run two time the program with valgrind with no randomness and the exact same time and valgrind told me this function take 10% of the program time in the first case and 3% in the second case. — thoregan, May 15 '17 at 11:54
@thoregan what where the optimization flags that you used to compile the code? — PeterT, May 15 '17 at 11:59
@Snps This is definitely about integer division, otherwise the computations would make no sense. — Sergey Kalinichenko, May 15 '17 at 12:02
Using a static object of non-POD type is bad practice as you have no control over object deinitialization time. — KonstantinL, May 15 '17 at 12:03
It is probably more common to calculate the position inside the chunk as `x % CHUNK_SIZE`. It [might even](http://stackoverflow.com/a/7070383/3422652) help the compiler optimize it into a single division. — Chris Drew, May 15 '17 at 12:49

Sergey Kalinichenko · Accepted Answer · 2017-05-15T12:01:15.433

In general, one should avoid using static when the only goal is to save CPU cycles.

Making coords static renders your map_coord_to_chunk_coord function non-reentrant, meaning that it is no longer usable in concurrent environments without proper synchronization. This is a very high price to pay for saving a construction cost of a simple object.

For example, you should be able to optimize construction of std::pair by using make_pair:

inline std::pair<sf::Vector2i, sf::Vector2i> 
Map::map_coord_to_chunk_coord(int x, int y) {
    int first_x = x / CHUNK_SIZE;
    int first_y = y / CHUNK_SIZE;
    return std::make_pair(
        sf::Vector2i(first_x, first_y)
    ,   sf::Vector2i(x - first_x * CHUNK_SIZE, y - first_y * CHUNK_SIZE)
    );
}

In certain cases the compiler can even optimize out the copying, further improving performance.

At least the way it is written in the question it does not save a construction, it adds one. — nwp, May 15 '17 at 11:55
@nwp I think it depends if the caller is assigning to an existing variable or not. — Chris Drew, May 15 '17 at 14:00

Chris Drew · Answer 2 · 2017-05-15T13:47:45.473

As others have pointed out you should generally avoid using local static variables in this way as it makes the code non thread safe.

Generally, the most idiomatic C++ is to rely on return-value-optimization and other compiler optimizations. I'd be surprised (and a little envious) if construction of a std::pair of sf::Vector2i was the bottleneck in your code but if this really is the critical piece of your code you could be slightly less idiomatic and pass-by-reference instead of using a return value:

void
map_coord_to_chunk_coord(int x, int y, std::pair<Vector2i, Vector2i>& chunk_coord) {
    chunk_coord.first.x = x / CHUNK_SIZE;
    chunk_coord.first.y = y / CHUNK_SIZE;
    chunk_coord.second.x = x % CHUNK_SIZE;
    chunk_coord.second.y = y % CHUNK_SIZE;
}

Then the caller can re-use a chunk_coord variable within a tight loop and you have no constructions of std::pair or sf::Vector2i.

score 1 · Answer 3 · answered May 15 '17 at 12:23

1

No.

You have to copy it anyway (return by value!), so you gained nothing.

(You actually added precisely one construction.)

All you achieved was making your function non-reentrant, which is a step backwards.

answered May 15 '17 at 12:23

Lightness Races in Orbit

378,754
76
643
1,055

C++ Static inside function

3 Answers3