[Freeciv-Dev] Re: Profiling Civserver again

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

Thue wrote:
> On Wednesday 25 July 2001 08:38, Jason Dorje Short wrote:

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

> I would prefer if you made a adjc_iterate_dir(center_x, center_y,
> itr_x, itr_y, itr_dir) macro. Then we could use it other places too,
> without having hand-optimizations spread all over the code.

Yes, that's a very good idea.  I had tried to fit this loop into the
adjc_iterate macro, and quickly realized it needs the directional value
as well.

> Actually I would prefer to put the optimization in a macro beside the
> other macros even if we only use it this once.

Well, for now the patch just changes this one loop.  However, it looks
like there are dozens of other loops that can also use the macro -
making the code both cleaner and faster.

> Oh, and I think it has been established that you don't want to use
> modulo :). As in the patch to normalize_map_pos() you want to use a
> conditional instead. Besides, all the tile will be adjacent to real
> ones, so you don't need a "while (x >= map.xsize)" like in
> normalize_map_pos(), just an "if (x >= map.xsize)".

Yes, I figured that out pretty quickly.  Actually it would make for the
cleanest code to use the map_adjust_x macro here instead; however, I
believe this code is faster than the macro will be because it assumes
the x value only needs to be adjusted by a little.  It wouldn't be a big
deal except that this code will be entered around a billion times in a
typical game.

Note that the loop itself is not particularly "optimized"; it still
checks both X and Y values 8 times each instead of the minimum 2 times
each.  Nonetheless, I believe the code as it is increases the speed of
really_generate_warmap by 5-15%.

jason

Index: common/map.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
retrieving revision 1.79
diff -u -r1.79 map.h
--- common/map.h        2001/07/01 20:54:00     1.79
+++ common/map.h        2001/07/25 09:48:06
@@ -364,6 +364,21 @@
   }                                                                           \
 }
 
+#define adjc_iterate_dir(center_x, center_y, itr_x, itr_y, itr_dir)           \
+{                                                                             \
+  int itr_x, itr_y, itr_dir;                                                  \
+  for (itr_dir = 0; itr_dir < 8; itr_dir++) {                                 \
+    itr_y = center_y + DIR_DY[itr_dir];                                       \
+    if (itr_y < 0 || itr_y > map.ysize) continue;                             \
+    itr_x = center_x + DIR_DX[itr_dir];                                       \
+    if (itr_x < 0) itr_x += map.xsize;                                        \
+    else if (itr_x >= map.xsize) itr_x -= map.xsize;                          \
+
+#define adjc_iterate_dir_end                                                  \
+  }                                                                           \
+}
+
+
 /* iterating y, x for cache efficiency */
 #define whole_map_iterate(WMI_x_itr, WMI_y_itr)                               \
 {                                                                             \
Index: 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 09:48:08
@@ -273,7 +273,7 @@
 void really_generate_warmap(struct city *pcity, struct unit *punit,
                            enum unit_move_type move_type)
 {
-  int x, y, move_cost, dir, x1, y1;
+  int x, y, move_cost;
   int orig_x, orig_y;
   int igter;
   int maxcost = THRESHOLD * 6 + 2; /* should be big enough without being TOO 
big */
@@ -307,12 +307,7 @@
 
   while (get_from_mapqueue(&x, &y)) {
     ptile = map_get_tile(x, y);
-    for (dir = 0; dir < 8; dir++) {
-      x1 = x + DIR_DX[dir];
-      y1 = y + DIR_DY[dir];
-      if (!normalize_map_pos(&x1, &y1))
-       continue;
-
+    adjc_iterate_dir(x, y, x1, y1, dir) {
       switch (move_type) {
       case LAND_MOVING:
        if (warmap.cost[x1][y1] <= warmap.cost[x][y])
@@ -363,7 +358,7 @@
        freelog(LOG_FATAL, "Bad/unimplemented move_type in 
really_generate_warmap().");
        abort();
       }
-    } /* end for */
+    } adjc_iterate_dir_end;
   }
 
   freelog(LOG_DEBUG, "Generated warmap for (%d,%d).",