[Freeciv-Dev] Re: Profiling Civserver again

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

Gregory Berkolaiko wrote:

> Since the most of the tiles are of the type "8" -- inner tiles, wouldn't
> it be simpler just to check for type 8 and if yes, don't do any more
> checks and if not, then do the normalize bit.
> 
> That is, in Jason's patch add
> 
> char is_border_tile=((center_y==0)||(center_y==map.ysize-1) ||
>                     (center_x==0)||(center_x==map.xsize-1));
> 
> before the for() loop and then inside the loop do
> 
> if (is_bother_tile) {
> /* do the renormalization thing */
> }
> 
> and that's it.
> Of course people might want to do some more profiling (sorry, cannot do it
> myself, I am away), but I am 95% sure that the above will be almost as
> fast as Trent's algorithm.

I'd guess it would be faster, since there's less overhead to gain the
same speedup for 95% of all tiles.

An updated patch will follow in a bit.


Vasco Alexandre Da Silva Costa 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.

Changing the (X % map.xsize < 0) to (X < 0) will not only be faster but
easier to read and (IMO) more correct.

> 
> 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;
> }

Actually, I think the original code may be faster.  Although the while()
loop has an effectively unbounded upper limit on the amount of time it
will take, in most (or possibly all?) cases it will only have to be
iterated once.  Of course, the function overhead is significant for such
a small function, but I'm not sure that that's worth it since with an
adjc_iterate_dir macro it would be called only a tiny fraction as
often.  I do think it's wise to change the code as you describe; it's
clearer and safer.


"Ross W. Wetmore" wrote:

> 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.

The macro does not need to use pointers, although it will be confusing
if it does not.  But as I said, I don't think it's necessary to move
this function into a macro.

jason