Second cut at opportunism.

Copied from Perforce
 Change: 37580
 ServerID: perforce.ravenbrook.com

mps/code/global.c

@@ -185,9 +185,7 @@ Bool GlobalsCheck(Globals arenaGlobals)
 
   CHECKL(arena->tracedSize >= 0.0);
   CHECKL(arena->tracedTime >= 0.0);
-  CHECKL(arena->savedStepTime >= 0.0);
-  CHECKL(arena->lastStep >= 0);
-  CHECKL(arena->lastCollected >= 0);
+  CHECKL(arena->lastWorldCollect >= 0);
 
   /* can't write a check for arena->epoch */
 
@@ -269,9 +267,7 @@ Res GlobalsInit(Globals arenaGlobals)
   arena->flippedTraces = TraceSetEMPTY; /* <code/trace.c> */
   arena->tracedSize = 0.0;
   arena->tracedTime = 0.0;
-  arena->savedStepTime = 0.0;
-  arena->lastStep = mps_clock();
-  arena->lastCollected = mps_clock();
+  arena->lastWorldCollect = mps_clock();
   arena->insideShield = FALSE;          /* <code/shield.c> */
   arena->shCacheI = (Size)0;
   arena->shCacheLimit = (Size)1;
@@ -558,13 +554,58 @@ void ArenaPoll(Globals globals)
 
   globals->insidePoll = TRUE;
 
-  (void)ArenaStep(globals, 0.0);
+  (void)ArenaStep(globals, 0.0, 0.0);
 
   globals->insidePoll = FALSE;
 }
 #endif
 
-Bool ArenaStep(Globals globals, double interval)
+/* Work out whether we have enough time here to collect the world,
+ * and whether much time has passed since the last time we did that
+ * opportunistically. */
+static Bool arenaShouldCollectWorld(Arena arena,
+                                    double interval,
+                                    double multiplier,
+                                    Word now,
+                                    Word clocks_per_sec)
+{
+  double scanRate;
+  Size arenaSize;
+  double arenaScanTime;
+  double sinceLastWorldCollect;
+
+  /* don't collect the world if we're not given any time */
+  if ((interval > 0.0) && (multiplier > 0.0)) {
+    /* don't collect the world if we're already collecting. */
+    if (arena->busyTraces == TraceSetEMPTY) {
+      /* don't collect the world if it's very small */
+      arenaSize = ArenaCommitted(arena) - ArenaSpareCommitted(arena);
+      if (arenaSize > 1000000) {
+        /* how long would it take to collect the world? */
+        if ((arena->tracedSize > 1000000.0) &&
+            (arena->tracedTime > 1.0))
+          scanRate = arena->tracedSize / arena->tracedTime;
+        else
+          scanRate = 25000000.0; /* a reasonable default. */
+        arenaScanTime = arenaSize / scanRate;
+        arenaScanTime += 0.1;   /* for overheads. */
+
+        /* how long since we last collected the world? */
+        sinceLastWorldCollect = ((now - arena->lastWorldCollect) /
+                                 (double) clocks_per_sec);
+        /* have to be offered enough time, and it has to be a long time
+         * since we last did it. */
+        if ((interval * multiplier > arenaScanTime) &&
+            sinceLastWorldCollect > arenaScanTime * 10.0) {
+          return TRUE;
+        }
+      }
+    }
+  }
+  return FALSE;
+}
+
+Bool ArenaStep(Globals globals, double interval, double multiplier)
 {
   double size;
   Size scanned;
@@ -575,16 +616,24 @@ Bool ArenaStep(Globals globals, double interval)
 
   AVERT(Globals, globals);
   AVER(interval >= 0.0);
+  AVER(multiplier >= 0.0);
 
   arena = GlobalsArena(globals);
   clocks_per_sec = mps_clocks_per_sec();
 
   start = mps_clock();
-  end = start + interval * clocks_per_sec;
+  end = start + (Word)(interval * clocks_per_sec);
   AVER(end >= start);
 
   stepped = FALSE;
 
+  if (arenaShouldCollectWorld(arena, interval, multiplier,
+                              start, clocks_per_sec)) {
+    ArenaStartCollect(globals);
+    arena->lastWorldCollect = start;
+    stepped = TRUE;
+  }
+
   /* loop while there is work to do and time on the clock. */
   do {
     scanned = TracePoll(globals);
@@ -597,61 +646,6 @@ Bool ArenaStep(Globals globals, double interval)
 
   if (stepped) {
     arena->tracedTime += (now - start) / (double) clocks_per_sec;
-    arena->savedStepTime = 0.0;
-    arena->lastStep = now;
-  } else if (interval > 0.0) {
-    /* All the CPU time since we last took a sensible step has been
-     * spent in the mutator.  If this is large, the mutator is busy
-     * and doing an opportunistic collect-world is not a good idea.
-     * But if it is small, and we have accumulated a lot of step time
-     * since then, then the mutator is offering us a lot of time in
-     * comparison to the amount of time it is taking, and can be
-     * considered idle for our purposes.
-     *
-     * Here 'large' and 'small' can be assessed by comparing amounts
-     * of time to the amount of time it would take to scan the arena.
-     *
-     * The problem here is spotting when we are idle.  Suppose that
-     * the mutator was busy a while ago, and there was a collection: a
-     * non-trivial step.  Then the mutator continued for a while but
-     * then went idle.  The continuing calls to mps_arena_step()
-     * continue to save up time but it never looks as if the mutator
-     * is really idle.
-     *
-     * So we need a better heuristic.  If we save up enough time, we
-     * can probably kick off a collection anyway, regardless of
-     * apparent mutator busyness.
-     */
-    double scanRate;
-    Size arenaSize;
-    double arenaScanTime;
-    double sinceLastStep;
-
-    arena->savedStepTime += interval;
-
-    if ((arena->tracedSize > 1000000.0) &&
-        (arena->tracedTime > 1.0))
-      scanRate = arena->tracedSize / arena->tracedTime;
-    else
-      scanRate = 25000000.0; /* a reasonable default. */
-
-    arenaSize = ArenaCommitted(arena) - ArenaSpareCommitted(arena);
-    arenaScanTime = arenaSize / scanRate;
-    if (arenaScanTime < 1.0)
-      arenaScanTime = 1.0;     /* clamp below to avoid being too eager */
-    sinceLastStep = (now - arena->lastStep) / (double) clocks_per_sec;
-    if ((arena->lastStep > arena->lastCollected) &&
-        ((arena->savedStepTime > arenaScanTime * 4.0) ||
-         ((arena->savedStepTime > arenaScanTime) &&
-          ((sinceLastStep < arenaScanTime / 10.0))))) {
-      /* either we've accumulated masses of step time since the last
-       * step, or the mutator seems idle and we've accumulated quite a
-       * bit. */
-      ArenaStartCollect(globals);
-      arena->savedStepTime = 0.0;
-      arena->lastStep = now;
-      arena->lastCollected = now;
-    }
   }
 
   size = globals->fillMutatorSize;

mps/code/mpm.h

@@ -471,7 +471,7 @@ extern void (ArenaPoll)(Globals globals);
 /* .nogc.why: ScriptWorks doesn't use MM-provided incremental GC, so */
 /* doesn't need to poll when allocating. */
 
-extern Bool (ArenaStep)(Globals globals, double interval);
+extern Bool (ArenaStep)(Globals globals, double interval, double multiplier);
 extern void ArenaClamp(Globals globals);
 extern void ArenaRelease(Globals globals);
 extern void ArenaPark(Globals globals);

mps/code/mpmst.h

@@ -677,9 +677,7 @@ typedef struct ArenaStruct {
   /* policy fields */
   double tracedSize;
   double tracedTime;
-  double savedStepTime;
-  Word lastStep;
-  Word lastCollected;
+  Word lastWorldCollect;
 
   RingStruct greyRing[RankLIMIT]; /* ring of grey segments at each rank */
   STATISTIC_DECL(Count writeBarrierHitCount); /* write barrier hits */

mps/code/mps.h

@@ -240,7 +240,7 @@ extern void mps_arena_release(mps_arena_t);
 extern void mps_arena_park(mps_arena_t);
 extern mps_res_t mps_arena_start_collect(mps_arena_t);
 extern mps_res_t mps_arena_collect(mps_arena_t);
-extern mps_bool_t mps_arena_step(mps_arena_t, double);
+extern mps_bool_t mps_arena_step(mps_arena_t, double, double);
 extern void mps_space_clamp(mps_space_t);
 extern void mps_space_release(mps_space_t);
 extern void mps_space_park(mps_space_t);

mps/code/mpsi.c

@@ -361,12 +361,14 @@ mps_res_t mps_arena_collect(mps_space_t mps_space)
   return res;
 }
 
-mps_bool_t mps_arena_step(mps_arena_t mps_arena, double time)
+mps_bool_t mps_arena_step(mps_arena_t mps_arena,
+                          double interval,
+                          double multiplier)
 {
   Bool b;
   Arena arena = (Arena)mps_arena;
   ArenaEnter(arena);
-  b = ArenaStep(ArenaGlobals(arena), time);
+  b = ArenaStep(ArenaGlobals(arena), interval, multiplier);
   ArenaLeave(arena);
   return b;
 }

mps/code/steptest.c

@@ -26,7 +26,7 @@
 #define objCOUNT          2000000
 #define clockSetFREQ      10000
 #define multiStepFREQ     500000
-#define multiStepSTEPS    100
+#define multiStepMULT     100
 
 #define genCOUNT          3
 #define gen1SIZE          750  /* kB */
@@ -273,11 +273,11 @@ static mps_addr_t make(void)
 
 /* call mps_arena_step() */
 
-static void test_step(mps_arena_t arena)
+static void test_step(mps_arena_t arena, double multiplier)
 {
     mps_bool_t res;
     double t1 = my_clock();
-    res = mps_arena_step(arena, 0.1);
+    res = mps_arena_step(arena, 0.1, multiplier);
     t1 = time_since(t1);
     if (res) {
         if (t1 > max_step_time)
@@ -301,7 +301,7 @@ static void *test(void *arg, size_t s)
     mps_chain_t chain;
     mps_root_t exactRoot, ambigRoot;
     unsigned long objs;
-    size_t i, j;
+    size_t i;
     mps_message_t message;
     size_t live, condemned, not_condemned;
     size_t messages;
@@ -376,11 +376,10 @@ static void *test(void *arg, size_t s)
         ++objs;
 
         if (objs % step_frequencies[test_number] == 0)
-            test_step(arena);
+            test_step(arena, 0.0);
 
         if (objs % multiStepFREQ == 0)
-            for (j=0; j<multiStepSTEPS; ++j)
-                test_step(arena);
+            test_step(arena, multiStepMULT);
 
         if (objs % clockSetFREQ == 0)
             set_clock_timing();