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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sat, Sep 01, 2001 at 06:09:40AM -0400, Jason Dorje Short wrote:
> 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.

I know.

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

I still like the idea of a is_border_position better than to do code
knowledge about the topologies into the iter macros (see Gautes point
from above). AFAI see normalize_map_pos can/will become more
complicated if other topologies are introduced. There is no nice way
to code normalize_map_pos later as a macro.

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

My current position: I agree the substituting is a good thing. I also
think the performance can be done later (there is enough potential). A
first step of performance could be the introduction of
is_border_map_pos.

I disagree with the inline function/macro thing. But this is a
seperate issue.

Do you agree Gaute? I would apply the patch in its current form.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Life is too short for reboots."