[Freeciv-Dev] Re: [PATCH] map_adjust_* patch

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Sat, 01 Sep 2001, jshort@xxxxxxxxxxxxx wrote:
> 
> The slowdown here is for one reason only: map_adjust_* is a macro
> and normalize_map_pos is not.  If normalize_map_pos were rewritten
> as a macro, this speed difference should be undone (in fact the
> compiled code should be identical in most cases).  Certainly there
> is no reason for the iteration macros to use map_adjust_* when they
> could just as easily use a normalization macro instead.

Hear, hear.  I agree completely, except that I would like it to be an
inline function rather than a macro.  It would be extremely hard to
write a macro that returns a value and implements looping constructs
at the same time.

> This is not a good reason to reject any part of the patch, since (as
> Gaute has pointed out) it should be easy to convert between the
> function and macro forms later.

I think the (rather large, if Raimar's measurements are anything to go
by) performance hit is a good reason to not apply the patch.  However,
if we all agree that an inline function is the way to go then we can
apply the patch now and change normalize_map_pos() later.

> If speed is that much of an issue, then the macro is definitely the
> way to go.  We've heard that most/all compilers won't be able to
> inline most functions (cross-source inlining), and this is not
> likely to change.  I don't think it's particularly worth it, but I
> won't complain (any more).

It sounds like you do not know very well what sort of optimizations C
compilers can and cannot perform.  The reason that normalize_map_pos()
is slower is not just the addition of function call overhead.  Since
we're taking the address of the two arguments, they have to stay in
memory (rather than a CPU register) for the rest of their lifetime.
After all, the compiler has no way of knowing that normalize_map_pos()
does not save these pointers anywhere and so the compiler must assume
that any function that it subsequently calls may change or read these
variables.  This basically eliminates the possibility of performing
any sort of optimisation on the loops that use these variables.  In
fact, the method that normalize_map_pos() uses is faster than that of
map_adjust_x(), so the difference is larger than you think.

> I also think is_border_map_pos is a necessary function/macro: it
> must replace the identical code that is already in adjc_dir_iterate
> (and which saves a LOT of time).  Again, the compiled code should be
> identical (assuming it's a macro, that is); however the
> topology-dependent parts will have been moved out into the
> function/macro.

Off hand I can only think of one obvious place where this is useful,
namely the one that you just mentioned.  is_border_pos() only makes
sense as a performance gain if you're looking at all the neighbours of
a tile; i.e. adjc_iterate() or adjc_dir_iterate().  The only valid
reason for going through the neighbours of a tile and not using
adjc_[dir_]iterate is that you really want to consider off-map
positions.  In that case, I suspect that you're better off using
MAPSTEP() and letting the compiler unhoist the appropriate checks from
the loop anyway.  This also has the advantage of not burdening the
local code with topological knowledge.

>> > Also it would be nice if you could convert the places that you
>> > touched to use map_inx() where appropriate.
>> 
>> Nice but not necessary.
> 
> This patch is *only* replacing map_adjust_* with normalize_map_pos.

I wasn't criticising; just pointing out a way that you could make
Freeciv better.  This qualifies as an obvious local bugfix; there is
no reason not to do it in passing anyway.

> I've tried to avoid (for now) substitution in code that needs a lot
> of cleanup so as to avoid conflict with other patches (particularly
> Ross's).

Nice try, but you may have noticed the qualification "the places that
you touched" above so I don't see how it applies.

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
C'MON, everybody!!  I've flown in LESLIE GORE and two dozen KOSHER
 BUTCHERS!  They'll be doing intricate MILITARY MANEUVERS to the
 soundtrack from "OKLAHOMA"!!