[Freeciv-Dev] Re: (PR#11828) patch: partial city update for what-if scenarios

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

Vasco Alexandre da Silva Costa wrote:

> Don't wait for an extension of the design to commit this patch if you
> think it is good and makes things faster though.

Well, according to Benoit's numbers it definitely makes things faster - 
which you'd expect, since it cuts out the biggest part of the refresh. 
The question is whether this is the best way to do it.

Another way would be by caching the tile output values and using the 
cached values if a certain parameter is given to the refresh.  This 
patch does that.  It decreases the CM time with the 10203 savegame on 
Boston from 2200 ms to 840 ms (this is with profiling enabled, which may 
affect the measurement), at a cost of 150 bytes (5-10%) extra memory 
used in the city struct.

These two methods accomplish more or less the same thing (avoiding 
recalculation of tile output) but with very different mechanisms.  The 
cache patch also avoids recalculation of the bonus[] array, which can be 
done separately with the what-if patch.

-jason

? diff
? gmon.out
Index: common/city.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/city.c,v
retrieving revision 1.303
diff -u -r1.303 city.c
--- common/city.c       8 Jan 2005 09:44:39 -0000       1.303
+++ common/city.c       10 Jan 2005 19:24:08 -0000
@@ -1649,15 +1649,12 @@
 static inline void get_worked_tile_output(const struct city *pcity,
                                          int *output)
 {
-  bool is_celebrating = base_city_celebrating(pcity);
-
   memset(output, 0, O_COUNT * sizeof(*output));
   
   city_map_iterate(x, y) {
-    if (get_worker_city(pcity, x, y) == C_TILE_WORKER) {
+    if (pcity->city_map[x][y] == C_TILE_WORKER) {
       output_type_iterate(o) {
-       output[o] += base_city_get_output_tile(x, y, pcity,
-                                              is_celebrating, o);
+       output[o] += pcity->tile_output[x][y][o];
       } output_type_iterate_end;
     }
   } city_map_iterate_end;
@@ -1705,6 +1702,24 @@
   } output_type_iterate_end;
 }
 
+/****************************************************************************
+  This function sets all the values in the pcity->tile_output[] array. This
+  should be called near the beginning of generic_city_refresh.  It doesn't
+  depend on anything else in the refresh and doesn't change when workers are
+  moved around (but does change when buildings are built, etc.).
+****************************************************************************/
+static inline void set_city_tile_output(struct city *pcity)
+{
+  bool is_celebrating = base_city_celebrating(pcity);
+
+  city_map_iterate(x, y) {
+    output_type_iterate(o) {
+      pcity->tile_output[x][y][o]
+       = base_city_get_output_tile(x, y, pcity, is_celebrating, o);
+    } output_type_iterate_end;
+  } city_map_iterate_end;
+}
+
 /**************************************************************************
   Set the final surplus[] array from the prod[] and usage[] values.
 **************************************************************************/
@@ -2140,16 +2155,34 @@
 }
 
 /**************************************************************************
-...
+  Refreshes the internal cached data in the city structure.
+
+  There are two possible levels of refresh: a partial refresh and a full
+  refresh.  A partial refresh is faster but can only be used in a few
+  places.
+
+  A full refresh updates all cached data: including but not limited to
+  ppl_happy[], surplus[], waste[], citizen_base[], usage[], trade[],
+  bonus[], and tile_output[].
+
+  A partial refresh will not update tile_output[] or bonus[].  These two
+  values do not need to be recalculated when moving workers around or when
+  a trade route has changed.  A partial refresh will also not refresh any
+  cities that have trade routes with us.  Any time a partial refresh is
+  done it should be considered temporary: when finished, the city should
+  be reverted to its original state.
 **************************************************************************/
 void generic_city_refresh(struct city *pcity,
-                         bool refresh_trade_route_cities,
+                         bool full_refresh,
                          void (*send_unit_info) (struct player * pplayer,
                                                  struct unit * punit))
 {
   int prev_tile_trade = pcity->citizen_base[O_TRADE];
 
-  set_city_bonuses(pcity);     /* Calculate the bonus[] array values. */
+  if (full_refresh) {
+    set_city_bonuses(pcity);   /* Calculate the bonus[] array values. */
+    set_city_tile_output(pcity); /* Calculate the tile_output[] values. */
+  }
   get_citizen_output(pcity, pcity->citizen_base); /* Calculate output from 
citizens. */
   set_city_production(pcity);
   citizen_happy_size(pcity);
@@ -2162,7 +2195,7 @@
   unhappy_city_check(pcity);
   set_surpluses(pcity);
 
-  if (refresh_trade_route_cities
+  if (full_refresh
       && pcity->citizen_base[O_TRADE] != prev_tile_trade) {
     int i;
 
@@ -2399,6 +2432,7 @@
   pcity->tile = ptile;
   sz_strlcpy(pcity->name, name);
   pcity->size = 1;
+  memset(pcity->tile_output, 0, sizeof(pcity->tile_output));
   specialist_type_iterate(sp) {
     pcity->specialists[sp] = 0;
   } specialist_type_iterate_end;
Index: common/city.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/city.h,v
retrieving revision 1.193
diff -u -r1.193 city.h
--- common/city.h       8 Jan 2005 05:14:17 -0000       1.193
+++ common/city.h       10 Jan 2005 19:24:08 -0000
@@ -223,6 +223,8 @@
   /* the people */
   int size;
 
+  unsigned char tile_output[CITY_MAP_SIZE][CITY_MAP_SIZE][O_MAX];
+
   /* How the citizens feel:
      ppl_*[0] is distribution before any of the modifiers below.
      ppl_*[1] is distribution after luxury.
@@ -496,7 +498,7 @@
 
 /* city update functions */
 void generic_city_refresh(struct city *pcity,
-                         bool refresh_trade_route_cities,
+                         bool full_refresh,
                          void (*send_unit_info) (struct player * pplayer,
                                                  struct unit * punit));
 void adjust_city_free_cost(int *num_free, int *this_cost);
Index: server/savegame.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/savegame.c,v
retrieving revision 1.216
diff -u -r1.216 savegame.c
--- server/savegame.c   1 Jan 2005 22:01:57 -0000       1.216
+++ server/savegame.c   10 Jan 2005 19:24:09 -0000
@@ -3464,10 +3464,7 @@
      * trade routes: the first refresh initializes all cities tile_trade
      * values; the second correctly updates all trade routes. */
     cities_iterate(pcity) {
-      generic_city_refresh(pcity, FALSE, NULL);
-    } cities_iterate_end;
-    cities_iterate(pcity) {
-      generic_city_refresh(pcity, FALSE, NULL);
+      generic_city_refresh(pcity, TRUE, NULL);
     } cities_iterate_end;
 
     /* Since the cities must be placed on the map to put them on the