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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Sep 02, 2001 at 06:30:11PM +0200, Gaute B Strokkenes wrote:
> 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.

Ack.

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

I think inline functions are the way to go. I think we can put them in
after we cleaned it up.

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

Interesting. I haven't thought of this.

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

Ack.

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

I consider such cases are rare and doesn't matter for performance.

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

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Just because you put a flag on the moon doesn't make it yours, it just
  puts a hole in the moon."