tresinformal / drakkar

The tresinformal video game called 'Drakkar'
GNU General Public License v3.0
11 stars 4 forks source link

C++ Core Guideline of October #636

Closed richelbilderbeek closed 1 year ago

richelbilderbeek commented 1 year ago

Probably 'Premature optimalization is the root of all evil' :-)

richelbilderbeek commented 1 year ago

Yep: Per.1: Don’t optimize without reason

Per.1: Don’t optimize without reason Per.2: Don’t optimize prematurely Per.3: Don’t optimize something that’s not performance critical Per.4: Don’t assume that complicated code is necessarily faster than simple code Per.5: Don’t assume that low-level code is necessarily faster than high-level code Per.6: Don’t make claims about performance without measurements Per.7: Design to enable optimization Per.10: Rely on the static type system Per.11: Move computation from run time to compile time Per.12: Eliminate redundant aliases Per.13: Eliminate redundant indirections Per.14: Minimize the number of allocations and deallocations Per.15: Do not allocate on a critical branch Per.16: Use compact data structures Per.17: Declare the most used member of a time-critical struct first Per.18: Space is time Per.19: Access memory predictably Per.30: Avoid context switches on the critical path

richelbilderbeek commented 1 year ago

Per.3: Don’t optimize something that’s not performance critical

This is a good example of clumsy code that probably does not need improvement:

  for (unsigned int i = 0; i != m_player.size(); ++i)
    {

      auto ID = std::to_string(i);
      coordinate player_position(
            300.0 + static_cast<unsigned int>(m_dist_x_pls) * i,
            400.0);
      m_player[i] =
          player(player_position,
                 player_shape::rocket,
                 player_state::active,
                 1,
                 -0.5,
                 0.1,
                 0.05,
                 0.1,
                 0.1,
                 100,
                 0.007,
                 color(i % 3 == 0 ? 255 : 0, i % 3 == 1 ? 255 : 0,
                       i % 3 == 2 ? 255 : 0),
                 ID);
    }

What can maybe be improved:

Here, m_player.size() requests the size of the vector. As the vector does not change in size, it is useless to request that size multiple times:

  for (unsigned int i = 0; i != m_player.size(); ++i)

Instead, do this:

  const unsigned int n{m_player.size()};
  for (unsigned int i = 0; i != n; ++i)

However, (1) maybe the compiler does this, (2) it is only 3 interations.

Taking out the size may be important to do nevertheless, not because of speed optimalization, but because it expresses what happens better!

Here is a similar example of rough code, where both i and j express themselves well:

  // For every projectile ...
  for (int i = 0 ; i != count_n_projectiles(*this) ; ++i)
  {
    //For every player...
    const int n_players = static_cast<int>(get_v_player().size());
    for(int j = 0; j != n_players; ++j)
    {
      // if it is not the one that shot it ...
      if(!(this->get_projectiles()[i].get_owner_id() == m_player[j].get_ID()))
     {
      double player_radius = m_player[j].get_diameter() / 2.0;
      // If the projectile touches the player ...
      if( this->m_projectiles[i].get_x() > this-> m_player[j].get_x() - player_radius &&
          this-> m_projectiles[i].get_x() < this-> m_player[j].get_x() + player_radius)
        {
          if(this-> m_projectiles[i].get_y() > this-> m_player[j].get_y() - player_radius &&
             this-> m_projectiles[i].get_y() < this-> m_player[j].get_y() + player_radius)
            {

              // if the projectile is a stun rocket: stun the player
              if(this-> m_projectiles[i].get_type() == projectile_type::stun_rocket)  {
                  this-> m_player[j].stun();

                  // projectile disappears
                  std::swap(m_projectiles[i], m_projectiles[m_projectiles.size()-1]);
                  this-> m_projectiles.pop_back();
                  return;
            }
          }
        }
      }
    }
  }
richelbilderbeek commented 1 year ago

Per.4: Don’t assume that complicated code is necessarily faster than simple code

A classical example, from https://en.wikipedia.org/wiki/XOR_swap_algorithm:

// C version
void XorSwap(int *x, int *y) 
{
  if (x != y)
  {
    *x ^= *y;
    *y ^= *x;
    *x ^= *y;
  }
}
// C++ equivalent
void XorSwap(int& x, int& y) 
{
  if (x != y)
  {
    x ^= y;
    y ^= x;
    x ^= y;
  }
}

This looks cool, but just use std::swap or:

void normal_swap(int& x, int& y)
{
  const int temp{x};
  x = y;
  y = temp;
}
richelbilderbeek commented 1 year ago

You can do TDD and optimize for speed:

const auto time_1{get_now()};
do_it();
const auto time_2{get_now()};
do_it_faster_maybe();
const auto time_3{get_now()};
const auto duration_1{time_2 - time_1};
const auto duration_2{time_3 - time_2};
assert(duration_2 < duration_1 * 0.1);

Be careful that the two tests must make sense, i.e. using big inputs!

richelbilderbeek commented 1 year ago

Per.6: Don’t make claims about performance without measurements

If you ever meet a person that says an array is faster than a std::vector in release mode, ask for code proving this. This usually highlights a misunderstanding, not the statement being true.

richelbilderbeek commented 1 year ago

Per.1: Don’t optimize without reason

Without any profiling information, we have no clue. We know our instincts are bad at predicting which code takes most runtime.

See https://github.com/richelbilderbeek/the_richel_setup#profile for an example profile.

richelbilderbeek commented 1 year ago

It's done :-)