[Freeciv-Dev] Re: Profiling Civserver again

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

I couldn't agree more with both of you, generic optimizations tend to
miss the mark and have a lot of potentially unseen side effects :-).

Kudos to Jason for actually following this through.

Jason's patch can probably be slightly improved upon by noting that
you can test for an edge case outside the inner loop and do either of
collecting his normalize map operations into a clause under a single
test, or cloning the inner loop into a tested and untested version.

In this case the untested version doesn't need to do anything (no map 
updates) so this reduces to a single test and branch, and cloning the
loop is probably not warranted :-).

Quick pseudo-code example below.

Cheers,
RossW
=====

  while (get_from_mapqueue(&x, &y)) {  
    ptile = map_get_tile(x, y);

    /* DIR_DX and DIR_DY adjustments only need to be 
     * tested if starting from a map border tile.
     * Include off map tiles for bugwards compatibility
     * or equivalently for idiot proofing.
     */
    boolean cond = (y <= 0 || y >= map.ysize-1 || x <= 0 || x >= map.xsize-1);
    for (dir = 0; dir < 8; dir++) {
      x1 = x + DIR_DX[dir];
      y1 = y + DIR_DY[dir];
      if( cond ) {
        if( y1 < 0 || y1 >= map.ysize )
          continue; 
        x1 = (x1 + map.xsize) % map.xsize;
      }
     ...

At 02:38 AM 01/07/25 -0400, Jason Dorje Short wrote:
>Trent Piepho wrote:
>> But really, you're just covering up the real inefficiency.  It would be
more
>> productive to figure out why map coordinates get normalized so many
times, and
>> reduce that number.
>
>I draw your attention to the following lines from the gprof run:
>
>[12]    20.4  100.19  102.49  159008         really_generate_warmap [12]
>               38.41    0.00 797302800/948124931     normalize_map_pos
>[32]
>
>This says that of the 950 million times normalize_map_pos was called 800
>million of them were in really_generate_warmap.  About 40% as much time
>was spent in normalize_map_pos as was spent in really_generate_warmap()
>itself.
>
>In other words, the following line accounts for about 4% of all Freeciv
>server CPU usage:
>       http://www.freeciv.org/lxr/source/server/gotohand.c?v=cvs#L313
>
>Aside from being astounding, this is totally unnecessary and can easily
>be improved on.
>
>With the attached patch, the use of normalize_map_pos is dropped and the
>normalization is handled manually.  This could be improved on much more
>- the normalization is still done for each of the 8 adjacent spots - but
>it is more efficient than the function call or even a standard macro
>would be.
>
>jasonIndex: server/gotohand.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/server/gotohand.c,v
>retrieving revision 1.99
>diff -u -r1.99 gotohand.c
>--- server/gotohand.c  2001/06/29 19:39:06     1.99
>+++ server/gotohand.c  2001/07/25 06:14:57
>@@ -308,9 +308,9 @@
>   while (get_from_mapqueue(&x, &y)) {
>     ptile = map_get_tile(x, y);
>     for (dir = 0; dir < 8; dir++) {
>-      x1 = x + DIR_DX[dir];
>+      x1 = (x + DIR_DX[dir] + map.xsize) % map.xsize;
>       y1 = y + DIR_DY[dir];
>-      if (!normalize_map_pos(&x1, &y1))
>+      if (y1 < 0 || y1 >= map.ysize)
>       continue;
> 
>       switch (move_type) {
>