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 ?

Answer 1

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.

Answer 2

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.