[Freeciv-Dev] Re: Profiling Civserver again

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

At 07:01 PM 01/07/25 -0400, Jason Dorje Short wrote:
>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

The macro *certainly* does or you need to change all the code occurences
that currently call the function with pointer arguments.

Vasco's initial step was to replace a function call with a macro to inline
the operations.

Cheers,
RossW

BTW, I should have been more careful with the ()'s to make sure that the 
* was outside as in *(X) and add an extra set in map_adjust, if someone
does use this. But I would recommend an inline function rather than a 
macro for this sort of thing to avoid risk.