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

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

Raimar Falke wrote:
> 
> On Sat, Sep 01, 2001 at 04:09:06AM +0200, Gaute B Strokkenes wrote:

> > I'm a bit ambivalent about using normalize_map_pos() inside iteration
> > macros like this one, and in low-level utility functions like
> > map_get_tile().  While it knocks the correctness issue out cold, I
> > worry about the overhead associated with a function call and
> > subsequent compiler optimizer confusion in these spots which are
> > executed an ungodly number of times during each ai turn.  IIRC
> > normalize_map_pos(), map_get_tile() and a few others are responsible
> > for about 20% of the CPU usage when running an average all-ai game,
> > which is already ridiculously much.  I think it's perfectly acceptable
> > for the iteration macros at least to know what topologies look like.
> >
> > If you profile a standard ai game both before and after applying your
> > patch and find no noticable slowdown, none of the above applies.  8-)
> 
> No profile but overall run time:

> unpatched:

[fast]

> patched:

[slower]

> I liked Ross's idea of a border position which would needs extra
> checking. Jason can you do such a patch?

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

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

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.

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

jason