[Freeciv-Dev] Re: Profile.

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Sat, 13 Oct 2001, vze2zq63@xxxxxxxxxxx wrote:
> 
> Yesterday I did some tests and achieved a 15% increase in autogame
> speed just by inlining normalize_map_pos.

Thanks for reminding me.  I've been meaning to do these sort of
measurements for a long time, but I have not had access to hardware
that makes this convenient until very recently.

I optimised is_real_tile() and normalize_map_pos() in various ways,
ran autogames and profiled them.  These are the results:

2001-10-14:             691.74s         0%
2001-10-14-irt:         665.38s         4% 
2001-10-14-nmp-gcc:     593.12s        14%
2001-10-14-nmp-hybrid:  626.60s         9%
2001-10-14-nmp-inline:  605.43s        12%
2001-10-14-nmp-macro:   593.35s        14%

2001-10-14 is plain CVS.

2001-10-14-irt is CVS with my patch from yesterday applied.

2001-10-14-nmp-gcc has macroised is_real_tile().  normalize_map_pos()
is implemented as a macro that expands into a statement expression,
which is a GCC extension.  It normalizes the x-coordinate by using two
while loops, which you can't do with a normal macro.

2001-10-14-nmp-inline implements is_real_tile() and normalize_map_pos()
just as now, except that both have been made static inline functions
in map.h.

2001-10-14-nmp-macro implements is_real_tile() as in -irt, and
normalize_map_pos as a macro that sets *x to map_adjust_x(*x).

2001-10-14-nmp-hybrid implements is_real_tile() as a static inline
function, and normalize_map_pos() as a static inline function that
sets *x to map_adjust_x(*x).

If anyone wants to have a look at the full profiles, they can find
them at:

  http://www.srcf.ucam.org/~gs234/gmon.out.2001-10-14.bz2
  http://www.srcf.ucam.org/~gs234/gmon.out.2001-10-14-irt.bz2
  http://www.srcf.ucam.org/~gs234/gmon.out.2001-10-14-nmp-gcc.bz2
  http://www.srcf.ucam.org/~gs234/gmon.out.2001-10-14-nmp-hybrid.bz2
  http://www.srcf.ucam.org/~gs234/gmon.out.2001-10-14-nmp-inline.bz2
  http://www.srcf.ucam.org/~gs234/gmon.out.2001-10-14-nmp-macro.bz2

I've also attached patches that show the changes that I made.  I would
not apply any of them as-is, but they exhibit the sort of changes that
can be made.

nmp-gcc.diff
Description: Text document

nmp-hybrid.diff
Description: Text document

nmp-inline.diff
Description: Text document

nmp-macro.diff
Description: Text document

Some observations:

  * Static inline functions are in all cases substantially slower than
    their macro counterparts.

  * There are substantial savings to be made.

  * The fastest versions are nmp-gcc and nmp-macro.  The GCC-specific
    version is faster by a negligible amount.

My recommendation would be to stick with a variant of nmp-macro.diff,
which is both portable and fast.

Off course, the best solution is to get rid of as many as possible
normalize_map_pos() calls and replace them either with a more
efficient idiom, such as MAPSTEP(), or to replace them with
assertion-like consistency checks (preferably fast consistency
checks).

> I don't *really* want to get into an inline vs macro argument again,
> but I think in the long run the solution is to make a new file,
> topology.h,

I am not convinced that there is a need to break things out from
map.h.

> and make all topology functions static inline within that file.
> This will make everything fast

Not very fast; as shown above, static inline is significantly slower
than a macro in these cases.

Also, you are assuming that all topologically emburdened functions
_must_ be inlined.  I propose that if we need to add further functions
to deal with these things then we simply implement them in the usual
manner and profile to see what we should and should not inline.

> while segregating the topology functions (all of which need to be
> extended when you add a new topology) 

I do not foresee a need for lots of new functions that need to know
about intricate topological details.  I also note that what we have
now is prefectly good for all the topologies that anyone is ever
likely to be interested in: rectangle, cylinder, torus, and their
isometric-view counterparts.

> and keeping everything pretty (having these functions be macros will
> be exceptionally ugly when more than one topology is implemented).

So you say, but I do not see how this follows.  is_real_tile() and
normalize_map_pos() both have obvious extensions to the cases
mentioned above.  If you want to implement one of the nightmare
topologies where normalisation is not a useful concept then you will
want to get rid of normalize_map_pos() in its entirety anyway.  That
makes it a bit lame to discuss exactly how it should be implemented.

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
Mr and Mrs PED, can I borrow 26.7% of the RAYON TEXTILE production
 of the INDONESIAN archipelago?