Fix GC so collection rate is proportional to alloc rate

- Previous comments in dobjgc.cpp suggested that StepMul was used
  to determine how quickly garbage was collected based on how quickly
  memory was being allocated. This was not the case. Now it is.
- Remove calls to CheckGC from the thinkers. With GC running at a
  stable rate (once per frame), there should be no need to inject
  pauses into the collection process to keep it from injecting stutters
  (provided StepMul is sane). The risk of running out of memory because
  we don't run a collection pass absolutely every thinker should be
  practically zero.
- Reduce DEFAULT_GCMUL from 400 to 200, since it does what it says now
  instead of something else.

src/common/objects/dobjgc.cpp

@@ -3,7 +3,7 @@
 ** The garbage collector. Based largely on Lua's.
 **
 **---------------------------------------------------------------------------
-** Copyright 2008 Randy Heit
+** Copyright 2008-2022 Marisa Heit
 ** All rights reserved.
 **
 ** Redistribution and use in source and binary forms, with or without
@@ -82,13 +82,19 @@
 ** infinity, where each step performs a full collection.) You can also
 ** change this value dynamically.
 */
-#define DEFAULT_GCMUL		400 // GC runs 'quadruple the speed' of memory allocation
+#define DEFAULT_GCMUL		200 // GC runs 'double the speed' of memory allocation
 
-// Number of sectors to mark for each step.
+// Minimum step size
+#define GCSTEPSIZE		(sizeof(DObject) * 16)
 
-#define GCSTEPSIZE		1024u
+// Maximum number of elements to sweep in a single step
 #define GCSWEEPMAX		40
-#define GCSWEEPCOST		10
+
+// Cost of sweeping one element (the size of a small object divided by
+// some adjust for the sweep speed)
+#define GCSWEEPCOST		(sizeof(DObject) / 4)
+
+// Cost of calling of one destructor
 #define GCFINALIZECOST	100
 
 // TYPES -------------------------------------------------------------------
@@ -99,6 +105,8 @@
 
 // PRIVATE FUNCTION PROTOTYPES ---------------------------------------------
 
+static size_t CalcStepSize();
+
 // EXTERNAL DATA DECLARATIONS ----------------------------------------------
 
 // PUBLIC DATA DEFINITIONS -------------------------------------------------
@@ -118,11 +126,15 @@ EGCState State = GCS_Pause;
 int Pause = DEFAULT_GCPAUSE;
 int StepMul = DEFAULT_GCMUL;
 int StepCount;
-size_t Dept;
+int CheckTime;
 bool FinalGC;
 
 // PRIVATE DATA DEFINITIONS ------------------------------------------------
 
+static int LastCollectTime;		// Time last time collector finished
+static size_t LastCollectAlloc;	// Memory allocation when collector finished
+static size_t MinStepSize;		// Cover at least this much memory per step
+
 // CODE --------------------------------------------------------------------
 
 //==========================================================================
@@ -161,8 +173,8 @@ size_t PropagateMark()
 //
 // SweepList
 //
-// Runs a limited sweep on a list, returning the location where to resume
+// Runs a limited sweep on a list, returning the position in the list just
-// the sweep at next time. (FIXME: Horrible Engrish in this description.)
+// after the last object swept.
 //
 //==========================================================================
 
@@ -254,6 +266,26 @@ void MarkArray(DObject **obj, size_t count)
 	}
 }
 
+//==========================================================================
+//
+// CalcStepSize
+//
+// Decide how big a step should be based, depending on how long it took to
+// allocate up to the threshold from the amount left after the previous
+// collection.
+//
+//==========================================================================
+
+static size_t CalcStepSize()
+{
+	int time_passed = CheckTime - LastCollectTime;
+	auto alloc = min(LastCollectAlloc, Estimate);
+	size_t bytes_gained = AllocBytes > alloc ? AllocBytes - alloc : 0;
+	return (StepMul > 0 && time_passed > 0)
+		? std::max<size_t>(GCSTEPSIZE, bytes_gained / time_passed * StepMul / 100)
+		: std::numeric_limits<size_t>::max() / 2;		// no limit
+}
+
 //==========================================================================
 //
 // MarkRoot
@@ -310,6 +342,10 @@ static void Atomic()
 	SweepPos = &Root;
 	State = GCS_Sweep;
 	Estimate = AllocBytes;
+
+	// Now that we are about to start a sweep, establish a baseline minimum
+	// step size for how much memory we want to sweep each CheckGC().
+	MinStepSize = CalcStepSize();
 }
 
 //==========================================================================
@@ -354,7 +390,8 @@ static size_t SingleStep()
 
 	case GCS_Finalize:
 		State = GCS_Pause;		// end collection
-		Dept = 0;
+		LastCollectAlloc = AllocBytes;
+		LastCollectTime = CheckTime;
 		return 0;
 
 	default:
@@ -374,29 +411,26 @@ static size_t SingleStep()
 
 void Step()
 {
-	size_t lim = (GCSTEPSIZE/100) * StepMul;
+	// We recalculate a step size in case the rate of allocation went up
-	size_t olim;
+	// since we started sweeping because we don't want to fall behind.
-	if (lim == 0)
+	// However, we also don't want to go slower than what was decided upon
-	{
+	// when the sweep began if the rate of allocation has slowed.
-		lim = (~(size_t)0) / 2;		// no limit
+	size_t lim = max(CalcStepSize(), MinStepSize);
-	}
-	Dept += AllocBytes - Threshold;
 	do
 	{
-		olim = lim;
+		size_t done = SingleStep();
-		lim -= SingleStep();
+		if (done < lim)
-	} while (olim > lim && State != GCS_Pause);
-	if (State != GCS_Pause)
-	{
-		if (Dept < GCSTEPSIZE)
 		{
-			Threshold = AllocBytes + GCSTEPSIZE;	// - lim/StepMul
+			lim -= done;
 		}
 		else
 		{
-			Dept -= GCSTEPSIZE;
+			lim = 0;
-			Threshold = AllocBytes;
 		}
+	} while (lim && State != GCS_Pause);
+	if (State != GCS_Pause)
+	{
+		Threshold = AllocBytes;
 	}
 	else
 	{
@@ -571,16 +605,13 @@ ADD_STAT(gc)
 		"  Sweep  ",
 		"Finalize " };
 	FString out;
-	out.Format("[%s] Alloc:%6zuK  Thresh:%6zuK  Est:%6zuK  Steps: %d",
+	out.Format("[%s] Alloc:%6zuK  Thresh:%6zuK  Est:%6zuK  Steps: %d  %zuK",
 		StateStrings[GC::State],
 		(GC::AllocBytes + 1023) >> 10,
 		(GC::Threshold + 1023) >> 10,
 		(GC::Estimate + 1023) >> 10,
-		GC::StepCount);
+		GC::StepCount,
-	if (GC::State != GC::GCS_Pause)
+		(GC::MinStepSize + 1023) >> 10);
-	{
-		out.AppendFormat("  %zuK", (GC::Dept + 1023) >> 10);
-	}
 	return out;
 }
 

src/common/objects/dobjgc.h

@@ -73,6 +73,9 @@ namespace GC
 	// Is this the final collection just before exit?
 	extern bool FinalGC;
 
+	// Counts the number of times CheckGC has been called.
+	extern int CheckTime;
+
 	// Current white value for known-dead objects.
 	static inline uint32_t OtherWhite()
 	{
@@ -111,6 +114,7 @@ namespace GC
 	static inline bool CheckGC()
 	{
 		AllocCount = 0;
+		CheckTime++;
 		if (AllocBytes >= Threshold)
 		{
 			Step();

src/playsim/dthinker.cpp

@@ -616,7 +616,6 @@ int FThinkerList::TickThinkers(FThinkerList *dest)
 			ThinkCount++;
 			node->CallTick();
 			node->ObjectFlags &= ~OF_JustSpawned;
-			GC::CheckGC();
 		}
 		node = NextToThink;
 	}
@@ -667,7 +666,6 @@ int FThinkerList::ProfileThinkers(FThinkerList *dest)
 			node->CallTick();
 			prof.timer.Unclock();
 			node->ObjectFlags &= ~OF_JustSpawned;
-			GC::CheckGC();
 		}
 		node = NextToThink;
 	}