[Freeciv-Dev] Re: (PR#11562) Freeciv Server Performance

["Per I. Mathisen" <per@xxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=11562 >

Here is a patch to start addressing the ugly problem of units being
managed twice each turn. This is a major CPU sink. I start addressing this
by introducing the punit->ai.done bool, which is set whenever a unit is
managed to satisfaction. This reduces the second loop by a third, roughly
speaking. I also add a comment that delineates my ideas for future
improvements. The patch finally contains some rather ugly debugging code,
which you may or may not want to test out if you are interested in this
part of the code.

One interesting find is that the second loop only actually moves a unit
that did not move in the first loop quite rarely. Around one in twenty
units, or so, roughly estimating, which means most players most of the
time never did.

Any comments welcome.

  - Per

Index: ai/aiferry.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiferry.c,v
retrieving revision 1.14
diff -u -r1.14 aiferry.c
--- ai/aiferry.c        19 Dec 2004 19:44:39 -0000      1.14
+++ ai/aiferry.c        22 Dec 2004 16:09:41 -0000
@@ -730,6 +730,7 @@
       && (pcity = map_get_city(punit->tile))) {
     UNIT_LOG(LOGLEVEL_FERRY, punit, "waiting in %s to recover hitpoints", 
              pcity->name);
+    punit->ai.done = TRUE;
     return;
   }
 
@@ -825,6 +826,7 @@
   if (IS_ATTACKER(punit) && punit->moves_left > 0) {
      /* AI used to build frigates to attack and then use them as ferries 
       * -- Syela */
+     ai_unit_new_role(punit, AIUNIT_ATTACK, NULL);
      UNIT_LOG(LOGLEVEL_FERRY, punit, "passing ferry over to attack code");
      ai_manage_military(pplayer, punit);
      return;
@@ -841,7 +843,7 @@
   if (aiferry_findcargo(punit)) {
     UNIT_LOG(LOGLEVEL_FERRY, punit, "picking up cargo (moves left: %d)",
             punit->moves_left);
-    ai_unit_goto(punit, punit->goto_tile);
+    (void) ai_unit_goto(punit, punit->goto_tile);
     return;
   }
 
@@ -849,22 +851,30 @@
   if (aiferry_find_interested_city(punit)) {
     if (same_pos(punit->tile, punit->goto_tile)) {
       UNIT_LOG(LOGLEVEL_FERRY, punit, "staying in city that needs us");
+      punit->ai.done = TRUE;
       return;
     } else {
       UNIT_LOG(LOGLEVEL_FERRY, punit, "going to city that needs us");
-      (void) ai_unit_goto(punit, punit->goto_tile);
+      if (ai_unit_goto(punit, punit->goto_tile)
+          && same_pos(punit->tile, punit->goto_tile)) {
+        punit->ai.done = TRUE; /* save some CPU */
+      }
       return;
     }
   }
 
   UNIT_LOG(LOGLEVEL_FERRY, punit, "Passing control of ferry to explorer code");
-  (void) ai_manage_explorer(punit);
+  if (!ai_manage_explorer(punit)) {
+    punit->ai.done = TRUE;
+  }
 
   if (find_unit_by_id(sanity) && punit->moves_left > 0) {
     struct city *pcity = find_nearest_safe_city(punit);
     if (pcity) {
       punit->goto_tile = pcity->tile;
       UNIT_LOG(LOGLEVEL_FERRY, punit, "No work, going home");
+      punit->ai.done = TRUE;
+      ai_unit_new_role(punit, AIUNIT_NONE, NULL);
       (void) ai_unit_goto(punit, pcity->tile);
     }
   }
Index: ai/aitools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aitools.c,v
retrieving revision 1.134
diff -u -r1.134 aitools.c
--- ai/aitools.c        19 Dec 2004 19:44:39 -0000      1.134
+++ ai/aitools.c        22 Dec 2004 16:09:42 -0000
@@ -55,6 +55,41 @@
 #include "aitools.h"
 
 /**************************************************************************
+  Return a string describing a unit's AI role.
+**************************************************************************/
+const char *get_ai_role_str(enum ai_unit_task task)
+{
+  switch(task) {
+   case AIUNIT_NONE:
+     return "None";
+   case AIUNIT_AUTO_SETTLER:
+     return "Auto settler";
+   case AIUNIT_BUILD_CITY:
+     return "Build city";
+   case AIUNIT_DEFEND_HOME:
+     return "Defend home";
+   case AIUNIT_ATTACK:
+     return "Attack";
+   case AIUNIT_FORTIFY:
+     return "Fortify";
+   case AIUNIT_RUNAWAY:
+     return "Run away";
+   case AIUNIT_ESCORT:
+     return "Escort";
+   case AIUNIT_EXPLORE:
+     return "Explore";
+   case AIUNIT_PILLAGE:
+     return "Pillage";
+   case AIUNIT_RECOVER:
+     return "Recover";
+   case AIUNIT_HUNTER:
+     return "Hunter";
+  }
+  assert(FALSE);
+  return NULL;
+}
+
+/**************************************************************************
   Amortize a want modified by the shields (build_cost) we risk losing.
   We add the build time of the unit(s) we risk to amortize delay.  The
   build time is claculated as the build cost divided by the production
Index: ai/aitools.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aitools.h,v
retrieving revision 1.49
diff -u -r1.49 aitools.h
--- ai/aitools.h        24 Nov 2004 19:14:12 -0000      1.49
+++ ai/aitools.h        22 Dec 2004 16:09:42 -0000
@@ -37,6 +37,8 @@
   BODYGUARD_NONE
 };
 
+const char *get_ai_role_str(enum ai_unit_task task);
+
 int military_amortize(struct player *pplayer, struct city *pcity, 
                       int value, int delay, int build_cost);
 int stack_cost(struct unit *pdef);
Index: ai/aiunit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.c,v
retrieving revision 1.342
diff -u -r1.342 aiunit.c
--- ai/aiunit.c 20 Dec 2004 23:50:58 -0000      1.342
+++ ai/aiunit.c 22 Dec 2004 16:09:43 -0000
@@ -816,8 +816,11 @@
   /* I had these guys set to just fortify, which is so dumb. -- Syela
    * Instead we can attack adjacent units and maybe even pick up some free 
    * cities! */
-  (void) ai_military_rampage(punit, BODYGUARD_RAMPAGE_THRESHOLD,
-                             RAMPAGE_FREE_CITY_OR_BETTER);
+  if (ai_military_rampage(punit, BODYGUARD_RAMPAGE_THRESHOLD,
+                          RAMPAGE_FREE_CITY_OR_BETTER)
+      && same_pos(punit->tile, ptile)) {
+    punit->ai.done = TRUE; /* Stay with charge */
+  }
 }
 
 /*************************************************************************
@@ -1783,6 +1786,7 @@
       /* This city needs defending, don't go outside! */
       UNIT_LOG(LOG_DEBUG, punit, "stayed to defend %s", 
                map_get_city(punit->tile)->name);
+      punit->ai.done = TRUE;
       return;
     }
 
@@ -1845,6 +1849,7 @@
     if (ai_manage_explorer(punit)) {
       UNIT_LOG(LOG_DEBUG, punit, "nothing else to do, so exploring");
     } else {
+      punit->ai.done = TRUE;
       UNIT_LOG(LOG_DEBUG, punit, "nothing to do - no more exploring either");
     }
   } else {
@@ -1871,11 +1876,13 @@
     struct city *pcity = map_get_city(punit->tile);
 
     if (pcity) {
+      punit->ai.done = TRUE;
       ai_unit_new_role(punit, AIUNIT_DEFEND_HOME, pcity->tile);
       /* FIXME: Send unit to nearest city needing more defence */
       UNIT_LOG(LOG_DEBUG, punit, "could not find work, sitting duck");
     } else {
       /* Going home */
+      punit->ai.done = TRUE;
       UNIT_LOG(LOG_DEBUG, punit, "sent home");
       /* FIXME: Rehome & send us to nearest city needing more defence */
       ai_military_gohome(pplayer, punit);
@@ -2006,6 +2013,8 @@
     UNIT_LOG(LOGLEVEL_RECOVERY, punit, "ready to kick ass again!");
     ai_unit_new_role(punit, AIUNIT_NONE, NULL);  
     return;
+  } else {
+    punit->ai.done = TRUE; /* sit tight */
   }
 }
 
@@ -2020,7 +2029,8 @@
   CHECK_UNIT(punit);
 
   /* "Escorting" aircraft should not do anything. They are activated
-   * by their transport or charge. */
+   * by their transport or charge.  We do _NOT_ set them to 'done'
+   * since they may need be activated once our charge moves. */
   if (punit->ai.ai_role == AIUNIT_ESCORT && is_air_unit(punit)) {
     return;
   }
@@ -2030,6 +2040,7 @@
       && ai_handicap(pplayer, H_AWAY)) {
     /* Don't move sentried or fortified units controlled by a player
      * in away mode. */
+    punit->ai.done = TRUE;
     return;
   }
 
@@ -2042,7 +2053,7 @@
   switch (punit->ai.ai_role) {
   case AIUNIT_AUTO_SETTLER:
   case AIUNIT_BUILD_CITY:
-    ai_unit_new_role(punit, AIUNIT_NONE, NULL);
+    assert(FALSE); /* This is not the place for this role */
     break;
   case AIUNIT_DEFEND_HOME:
     ai_military_gohome(pplayer, punit);
@@ -2054,6 +2065,7 @@
     ai_military_gohome(pplayer, punit);
     break;
   case AIUNIT_RUNAWAY: 
+    /* ??? */
     break;
   case AIUNIT_ESCORT: 
     ai_military_bodyguard(pplayer, punit);
@@ -2062,7 +2074,7 @@
     handle_unit_activity_request(punit, ACTIVITY_PILLAGE);
     return; /* when you pillage, you have moves left, avoid later fortify */
   case AIUNIT_EXPLORE:
-    (void) ai_manage_explorer(punit);
+    punit->ai.done = !(ai_manage_explorer(punit) && punit->moves_left > 0);
     break;
   case AIUNIT_RECOVER:
     ai_manage_hitpoint_recovery(punit);
@@ -2128,6 +2140,7 @@
   /* Don't manage the unit if it is under human orders. */
   if (unit_has_orders(punit)) {
     punit->ai.ai_role = AIUNIT_NONE;
+    punit->ai.done = TRUE;
     return;
   }
 
@@ -2135,15 +2148,10 @@
      function */
   if( is_barbarian(pplayer) ) {
     /* Todo: should be configurable */
-    if( unit_can_be_retired(punit) && myrand(100) > 90 ) {
+    if (unit_can_be_retired(punit) && myrand(100) > 90) {
       wipe_unit(punit);
       return;
     }
-    if( !is_military_unit(punit)
-       && !unit_has_role(punit->type, L_BARBARIAN_LEADER)) {
-      freelog(LOG_VERBOSE, "Barbarians picked up non-military unit.");
-      return;
-    }
   }
 
   /* Check if we have lost our bodyguard. If we never had one, all
@@ -2155,6 +2163,7 @@
 
   if (punit->moves_left <= 0) {
     /* Can do nothing */
+    punit->ai.done = TRUE;
     return;
   }
 
@@ -2185,6 +2194,7 @@
   } else if (is_heli_unit(punit)) {
     /* TODO: We can try using air-unit code for helicopters, just
      * pretend they have fuel = HP / 3 or something. */
+    punit->ai.done = TRUE; /* we did our best, which was ... nothing */
     return;
   } else if (is_military_unit(punit)) {
     ai_manage_military(pplayer,punit); 
@@ -2203,18 +2213,62 @@
 
 /**************************************************************************
   Master manage unit function.
+
+  IDEA: Master scheme. We first try to give each unit a role. Then we 
+  try three loops over the unit list, and hopefully by the end of the
+  third loop (or, really, by the end of the first), all units are either
+  set to 'done' or out of move points. Do _not_ rerun the role 
+  distribution between each iteration, as is done now, as this makes
+  the resetting of the role when a manage function fails to utilize a 
+  unit meaningless, and we will just try the same role again and with 
+  predictable results.
+
+  A manage function should set the unit to 'done' when it should no
+  longer be touched by this code, and its role should be reset to IDLE
+  when its role has accomplished its mission or the manage function
+  fails to have or no longer has any use for the unit.
 **************************************************************************/
 void ai_manage_units(struct player *pplayer) 
 {
+  int units = 0, saved = 0;
+
   ai_airlift(pplayer);
   unit_list_iterate_safe(pplayer->units, punit) {
+    punit->ai.done = FALSE;
     ai_manage_unit(pplayer, punit);
+    units++;
   } unit_list_iterate_safe_end;
+  {
+  int cache[units], ids[units], terrain[units], role[units], i = 0;
+  unit_list_iterate(pplayer->units, punit) {
+    ids[i] = punit->id;
+    cache[i] = punit->moves_left;
+    terrain[i] = punit->tile->terrain;
+    role[i] = punit->ai.ai_role;
+    i++;
+  } unit_list_iterate_end;
   /* Sometimes units wait for other units to move so we crudely
    * solve it by moving everything again */ 
-  unit_list_iterate_safe(pplayer->units, punit) {
-    ai_manage_unit(pplayer, punit);
-  } unit_list_iterate_safe_end;
+  for (i = 0; i < units; i++) {
+    struct unit *punit = find_unit_by_id(ids[i]);
+    if (punit && !punit->ai.done) {
+      ai_manage_unit(pplayer, punit);
+      if (find_unit_by_id(ids[i]) && punit->moves_left != cache[i]) {
+        UNIT_LOG(LOG_NORMAL, punit, "Moves left differ %d to %d; "
+                 "was on %s is now on %s; was %s is now %s", cache[i],
+                 punit->moves_left, get_terrain_name(punit->tile->terrain),
+                 get_terrain_name(terrain[i]), get_ai_role_str(role[i]),
+                 get_ai_role_str(punit->ai.ai_role));
+      }
+    } else if (find_unit_by_id(ids[i]) && punit->ai.done) {
+      saved++;
+    }
+  }
+  }
+  freelog(LOG_NORMAL, "saved %d (of %d) units from a second loop", saved, 
units);
+  /*
+  unit_has_role(aunit->type, L_BARBARIAN_LEADER) 
+  */
 }
 
 /**************************************************************************
Index: common/unit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/unit.c,v
retrieving revision 1.223
diff -u -r1.223 unit.c
--- common/unit.c       7 Dec 2004 18:01:12 -0000       1.223
+++ common/unit.c       22 Dec 2004 16:09:45 -0000
@@ -1726,6 +1726,7 @@
   if (is_barbarian(pplayer)) {
     punit->fuel = BARBARIAN_LIFE;
   }
+  punit->ai.done = FALSE;
   punit->ai.cur_pos = NULL;
   punit->ai.prev_pos = NULL;
   punit->ai.target = 0;
Index: common/unit.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/unit.h,v
retrieving revision 1.131
diff -u -r1.131 unit.h
--- common/unit.h       7 Dec 2004 18:01:12 -0000       1.131
+++ common/unit.h       22 Dec 2004 16:09:45 -0000
@@ -118,6 +118,7 @@
 
   int target; /* target we hunt */
   int hunted; /* if a player is hunting us, set by that player */
+  bool done;  /* we are done controlling this unit this turn */
 };
 
 struct unit {
Index: server/settlers.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/settlers.c,v
retrieving revision 1.216
diff -u -r1.216 settlers.c
--- server/settlers.c   19 Dec 2004 19:44:40 -0000      1.216
+++ server/settlers.c   22 Dec 2004 16:09:49 -0000
@@ -146,6 +146,7 @@
 void ai_manage_settler(struct player *pplayer, struct unit *punit)
 {
   punit->ai.control = TRUE;
+  punit->ai.done = TRUE; /* we will manage this unit later... ugh */
   /* if BUILD_CITY must remain BUILD_CITY, otherwise turn into autosettler */
   if (punit->ai.ai_role == AIUNIT_NONE) {
     ai_unit_new_role(punit, AIUNIT_AUTO_SETTLER, NULL);