[Freeciv-Dev] Re: Profiling Civserver again

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

Trent Piepho wrote:
> 
> On Thu, 26 Jul 2001, Jason Dorje Short wrote:
> > of map nodes (in total) can be found from the number of times
> > get_from_mapqueue() is called.  This divides into the total time spent
> > in the function - including time spent in called functions.
> >
> > I've done five runs.  The values are as follows:
> >
> > original code - one run - 1.5 us per map node
> > "better" code (my first patch) - 1.4, 1.3, and 1.4 us per map node
> > "best" code - one run - 1.2 us per map node
> 
> Did you try my version?  I think it will be faster. It's also cleaner, with
> each tile handled in the same manner and less code inside the iterator macro.

I haven't tried your version.  I disagree that it is cleaner - a 9x8
array of numbers is neither clean nor intuitive IMO.  Additionally, the
overhead necessary to initialize DIR_DX/DIR_DY after the map is set up
is not worth it, IMO.  You'll have to spread the code around into 3
places - one for the iterator macro, one for the declaration of
DIR_DX/DIR_DY, and one to initialize the values of DIR_DX/DIR_DY.

> I also think you named the macro incorrectly.  All the other iterator macros
> end with _iterate, but you ended it with _dir.

You're right.  I named it like this just because that's the name Thue
gave, and did the same thing with the arguments.  Neither are consistent
with the other similar macros - in fact none of these macros are
particularly consistent.  The attached patch slightly improves the
naming of the adjc_dir_iterate macro (leaving the others as-is).

> There are currently two sets of DIR_DX arrays.  One is called DIR_DX2 and
> differs in the order of the directions.  DIR_DX2 uses a more sensible
> method of numbering the tiles clockwise from 0.  It's only used one place
> in the tilespec code.  Which is too bad, because it makes more sense to
> order the tiles that way.

Yes, DIR_DX2 uses a more intelligent numbering system.  However, it will
be difficult to upgrade to use this data instead.  It's too bad DIR_DX
wasn't written like this from the start.

> The cartesian_adjacent_iterate macro could also use this method.  I'd rename
> it to adjc_cardinal_iterate too.  If the clockwise ordering is used for
> directions, almost the exact same code can be used for this macro as the
> adjc_iterate() macro.  The only difference is to increment dir by 2 each
> iteration.

True, but it could also use CAR_DIR_DX as well (which would be far
superior to the current setup).  It might be possible to do better by
making the cardinality an option to the macro, since I know there are
some places in the code where it is handled like this.  I'm not sure how
many places there are that are like that, though.

> It's too bad no one after me took the time to really clean up the code.
> People just keep doing crude hacks, like speeding up a single loop when there
> are dozens of identical loops that could be done the same way.  It's just
> cleaner to be consistent.  It's really quite depressing watching something
> you spent so long trying to write well get turned into a mess of kludges.

I think this is an unintended effect of the higher safety value placed
on the CVS code.  Patches generally aren't applied until they've
circulated for a while, and many circulate indefinitely.  I think patch
writers therefore try to make their patches as unintrusive and simple as
possible so that they have a better chance of being understood and
applied.  I agree that this is a bad thing.  Hopefully after the release
CVS will be opened up more and some of these things can be cleaned up -
and hopefully in the future CVS branches can be used so that the code
can still be improved while FreeCiv is in the 6-month process of a
release...

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/26 17:32:42
@@ -364,6 +364,27 @@
   }                                                                           \
 }
 
+/* Iterate through all tiles adjacent to a tile.  x_itr and y_itr are as
+ * in adjc_iterate; dir_itr is the directional value (see DIR_D[XY]). */
+/* This assumes that center_x and center_y are normalized. --JDS */
+#define adjc_dir_iterate(center_x, center_y, x_itr, y_itr, dir_itr)           \
+{                                                                             \
+  int x_itr, y_itr, dir_itr, border;                                          \
+  border = (!center_y || !center_x ||                                         \
+           (center_y == map.ysize-1) || (center_x == map.xsize-1));           \
+  for (dir_itr = 0; dir_itr < 8; dir_itr++) {                                 \
+    y_itr = center_y + DIR_DY[dir_itr];                                       \
+    x_itr = center_x + DIR_DX[dir_itr];                                       \
+    if (border) {                                                             \
+      if (y_itr < 0 || y_itr > map.ysize) continue;                           \
+      if (x_itr < 0) x_itr += map.xsize;                                      \
+      else if (x_itr >= map.xsize) x_itr -= 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/26 17:32:44
@@ -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_dir_iterate(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).",