[Freeciv-Dev] [PATCH] Small improvement at genlist and maphand

[Markus Linnala <maage@xxxxxxxxx>]

I propose we change genlist iterator macros as follows. This
allows you two things. You can get rid of extra function call.
And you can also inline call. As patch follows.

genlist_iterator_init is called more than 99% by these macros.
Therefore we should optimize fast track.

------------------------------------------------------------------------
diff -u -r1.6 genlist.h
--- common/genlist.h    2000/01/26 01:03:27     1.6
+++ common/genlist.h    2001/08/20 07:46:04
@@ -110,6 +110,10 @@
 void genlist_iterator_init(struct genlist_iterator *iter,
                                struct genlist *pgenlist, int pos);
 
+static inline void genlist_iterator_init_m1(struct genlist_iterator *iter,
+                                           struct genlist *pgenlist);
+static inline void genlist_iterator_init_0(struct genlist_iterator *iter,
+                                          struct genlist *pgenlist);
 #define ITERATOR_PTR(X)  ((X).link->dataptr)
 #define ITERATOR_NEXT(X) ((X).link=(X).link->next)
 #define ITERATOR_PREV(X) ((X).link=(X).link->prev)
@@ -123,7 +127,7 @@
 #define TYPED_LIST_ITERATE(atype, typed_list, var) {       \
   struct genlist_iterator myiter;                          \
   atype *var;                                              \
-  genlist_iterator_init(&myiter, &(typed_list).list, 0);   \
+  genlist_iterator_init_0(&myiter, &(typed_list).list);   \
   for(; ITERATOR_PTR(myiter);) {                           \
     var=(atype *)ITERATOR_PTR(myiter);                     \
     ITERATOR_NEXT(myiter);
@@ -136,7 +140,7 @@
 #define TYPED_LIST_ITERATE_REV(atype, typed_list, var) {   \
   struct genlist_iterator myiter;                          \
   atype *var;                                              \
-  genlist_iterator_init(&myiter, &(typed_list).list, -1);  \
+  genlist_iterator_init_m1(&myiter, &(typed_list).list);  \
   for(; ITERATOR_PTR(myiter);) {                           \
     var=(atype *)ITERATOR_PTR(myiter);                     \
     ITERATOR_PREV(myiter);
@@ -144,5 +148,19 @@
 /* Balance for above: */ 
 #define LIST_ITERATE_REV_END  }}
 
+#if 1
+void genlist_iterator_init_m1(struct genlist_iterator *iter,
+                               struct genlist *pgenlist)
+{
+  iter->list=pgenlist;
+  iter->link=pgenlist->tail_link;
+}
+void genlist_iterator_init_0(struct genlist_iterator *iter,
+                               struct genlist *pgenlist)
+{
+  iter->list=pgenlist;
+  iter->link=pgenlist->head_link;
+}
+#endif
 
 #endif  /* FC__GENLIST_H */
------------------------------------------------------------------------


Same kind of optimization goes to get_player. It is not used
everywhere by the way.

Now we do something like that.

void function_xx(struct player *pplayer, int x, int y) {
        int player_no = pplayer->player_no;
        struct tile *ptile = map_get_player_tile(x, y, player_no);
....
}
struct player_tile *map_get_player_tile(int x, int y, int playerid)
{
  if(y<0 || y>=map.ysize) {
    freelog(LOG_ERROR, "Trying to get nonexistant tile at %i,%i", x,y);
    return get_player(playerid)->private_map
      + map_adjust_x(x)+map_adjust_y(y)*map.xsize;
  } else
    return get_player(playerid)->private_map
      + map_adjust_x(x)+y*map.xsize;
}

And at map_get_player_tile, we find out again what is struct
player. Almost all use of get_player is like this. First we have
'struct player', then we get player_no and in next instant we
get 'struct player' again by player_no. Seems waste of work to
me.

Here is some data from profiler.
------------------------------------------------------------------------
  %   cumulative   self              self     total           
 time   seconds   seconds    calls  ms/call  ms/call  name    
 11.47      4.22     4.22       22   191.82   330.72  check_fow
  8.29      7.27     3.05   602241     0.01     0.01  sync_cities
  7.75     10.12     2.85 130288264     0.00     0.00  get_player
------------------------------------------------------------------------

I think map_get_player should be something like this:

struct player_tile *map_get_player_tile(int x, int y, struct player *pplayer)
{
  if(y<0 || y>=map.ysize) {
    freelog(LOG_ERROR, "Trying to get nonexistant tile at %i,%i", x,y);
  }
  return pplayer->private_map
    + map_adjust_x(x)+map_adjust_y(y)*map.xsize;
}

And after that change calls accordingly.

-- 
//Markus