[Freeciv-Dev] Re: Profiling Civserver again

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

At 09:12 PM 01/07/31 +0200, Thue wrote:
>> Another point of note is that on each and every call it normalizes
>> the map pos, even though for most calls this is already known. 
>> Introducing a second function/macro, map_get_normal_tile(x, y) would
>> give some savings here (you'd still want to assert that the tile was
>> normal), but I don't see an advantage otherwise.
>
>I think the easiest way is to just always use normalized tile coords. I 
>think this is already the case in most of the code (I have been 
>cleaning it up for some time).
>
>-Thue

Personally I prefer doing something like the following in map.h ...

#ifndef MAP_GET_TILE_FUNCTION

/* WARNING: these are macros - do not use arguments with side effects */
#define map_get_tile(x,y)       ( \
  ((unsigned)(y) >= (unsigned)map.ysize)  \
  ? &void_tile               \
  : map_get_tile_unchecked(map_adjust_x(x),(y)))

#define map_get_tile_unchecked(x,y)     ( \
  map_get_tile1_unchecked(map.xsize*(y)+(x)))

#define map_get_tile1_unchecked(xy)     ( \
     map.tiles+(xy))

#else

extern struct tile *map_get_tile(int x, int y);

#endif /* MAP_GET_TILE_FUNCTION */

And then go through the code replacing map_get_tile calls with the 
appropriate unchecked macro as they are confirmed safe, and flagged
by profiling.

If one wants to be a little more risk agressive and *not* remind the
developer of the *unchecked* danger, name the three functions in order:
map_get_tile_checked(x,y), map_get_tile(x,y) and map_get_tile1(xy). 
Then go through the code changing all those that are deemed to be 
*unsafe*.

In either case you may find local optimizations to reduce the two 
coordinate computation.

[-: Mount Soapbox ...
There should always be single coordinate versions as well as double
ones for just such cases. And doing things like the above makes it
easy for compilers to recognize common code to optimize out when you
haven't spent the effort to optimize by hand.
 ... Dismount Soapbox :-]

You can do the same sort of thing for a number of other common objects
accessed through map coordinates, or build a generic flavour like the
following that can be used anywhere.

#define map_get_object(obj,default_obj,x,y)     (  \
  ((unsigned)(y) >= (unsigned)map.ysize)  \
  ? (def_obj) \
  : map_get_object_unchecked((obj),map_adjust_x(x),(y)))

#define map_get_object_unchecked(obj,x,y)       ( \
  map_get_object1_unchecked((obj),map.xsize*(y)+(x)))

#define map_get_object1_unchecked(obj,xy)       ( \
     (obj)+(xy))

Gotchas:
There is an added restriction that only expressions with no side effects,
and generally only fully evaluated expressions should be used in the
macro form which should be doc'd as above to refresh the reminder.

Cheers,
RossW