[Freeciv-Dev] Re: Profiling Civserver again

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 02:50 AM 01/07/25 +0100, you wrote:
>Looking at the code some more functions can be inlined. I also noticed
>somethings: for e.g.
>
>=== map_adjust_x(X) on map.h:
>The '(X) % map.xsize < 0' conditional can be replaced with '(X) < 0' which
>is faster and is correct due to the following propositions being true (or
>so i think):
>
>the integer remainder operation '%' only provides negative results when
>either the dividend or the divisor is negative.
>map.xsize is always >= 0.
>
>1264 int normalize_map_pos(int *x, int *y) {
>1265   if (*y < 0 || *y >= map.ysize)
>1266     return FALSE;
>1267 
>1268   while (*x < 0)
>1269     *x += map.xsize;
>1270   while (*x > map.xsize-1)
>1271     *x -= map.xsize;
>1272 
>1273   return TRUE;
>1274 }
>
>Is grossly inneficient. This code should be equivalent:
>int normalize_map_pos(int *x, int *y) {
>  if (*y < 0 || *y >= map.ysize)
>    return FALSE;
>  *x = map_adjust_x(*x);
>  return TRUE;
>}
>
>Still, looking at the results of the run that Paul provided, given that
>this accounts for more than 4% of running time this function should be
>inlined somehow. It really should be turned into some kind of macro since
>all these pointers and the function call overheard for such a simple
>function cause the slowdown.
>
>This should work:
>#define normalize_map_pos(X, Y) \
>  ((X)=map_adjust_x(X), ((Y) >= 0 && (Y) < map.ysize))
>
>But i guess it could be more efficient.
>
>---
>Vasco Alexandre da Silva Costa @ Instituto Superior Tecnico, Lisboa

Technically this is slightly better.

#define normalize_map_pos(X, Y) \
  (((unsigned)(*Y) < (unsigned)map.ysize) && ((*X)= map_adjust_x(*X), TRUE))

1)  The function (and macro) uses pointers because it modifies the x value.
2)  A FALSE Y condition should leave x unchanged, faster anyway this way.
3)  You can squeeze out a compare by using a single unsigned compare.

Note: possible traps laid for you by Murphy
1)  Y is used only once so it could be an expression - X has to be a pointer
    expression, and it is hopefully unlikely that the multiple invocations
    will do bad things, but someone might have put a "px++" in the arg list.
2)  TRUE, map and map_adjust_x() will need to be defined in all functions
    that call the macro, whereas they weren't for the function call.

Cheers,
RossW
=====