Subject: Re: bug#79346: igc: PolicyShouldCollectWorld

From: Helmut Eller <[email-masked]>
Date: Thu, 04 Sep 2025 22:05:55 +0200

On Thu, Sep 04 2025, Gerd Möllmann wrote:

> Helmut Eller <[email-masked]> writes:
[...]
>> What would you think of making mps_arena_step and perhaps buffer_step
>> available from Lisp so that those things can be called from Lisp-level
>> idle timers?
>
> Sounds like a good idea.

Below is a patch that does this.

The idle timer waits 2 seconds, before it calls mps_arena_step.  As
usual, "idle time" is the time since the last command completed, i.e. 2
seconds must pass after a command before the idle timer fires.  (Other
timers or process filters don't reset "idle time"; only new commands
do.)  I think that should avoid the problem that I've seen where
opportunistic GC was started while I was in the middle of typing
something.

The code uses a constant to predict the idle time; maybe one could to
something a bit more clever; not sure if that would be worth the
trouble.

Regarding bug#76505 (or https://github.com/Ravenbrook/mps/issues/305),
I now think that it's safe to call mps_arena_step with multiplier=0:
The expression (availableEnd - now) never overflows because the
invariant now <= intervalEnd <= availableEnd holds.

>From 629fef5cf92e6232288efb4c81097649da632e46 Mon Sep 17 00:00:00 2001
From: Helmut Eller <[email-masked]>
Date: Thu, 4 Sep 2025 21:04:40 +0200
Subject: [PATCH] Call mps_arena_step from a Lisp idle timer

* src/igc.c (Figc__arena_step): New defun.
(arena_step, Vigc_step_interval): Deleted.
(igc_on_idle): Don't call arena_step.
(syms_of_igc): Register Sigc__arena_step.
* lisp/emacs-lisp/igc.el (igc-start-idle-timer): New.
(igc--idle-timer, igc--idle-delay, igc--step-interval)
(igc--predict-idle-time, igc--on-idle): New code for opportunistic Gc.
---
 lisp/emacs-lisp/igc.el | 33 +++++++++++++++++++++
 src/igc.c              | 65 +++++++++++++++++++-----------------------
 2 files changed, 62 insertions(+), 36 deletions(-)

diff --git a/lisp/emacs-lisp/igc.el b/lisp/emacs-lisp/igc.el
index de7aae9f4ee..f306db99a93 100644
--- a/lisp/emacs-lisp/igc.el
+++ b/lisp/emacs-lisp/igc.el
@@ -388,6 +388,39 @@ igc-start-collecting-stats
          (setq igc--collect-timer
                (run-at-time nil secs #'igc--collect-stats-sqlite)))))
 
+
+;;; Opportunistic GC
+
+(defvar igc--idle-timer nil
+  "Idle timer to trigger oppurtunistic GC.")
+
+(defvar igc--idle-delay 2.0
+  "Time, in seconds, to wait for `igc--idle-timer'.")
+
+(defvar igc--step-interval 0.01
+  "Time, in seconds, MPS is allowed to use for one step.")
+
+(defun igc-start-idle-timer ()
+  "Start a timer to do GC work while Emacs is idle."
+  (when igc--idle-timer
+    (cancel-timer igc--idle-timer))
+  (setq igc--idle-timer
+        (run-with-idle-timer igc--idle-delay t #'igc--on-idle)))
+
+(defun igc--predict-idle-time ()
+  (* igc--idle-delay 0.66))
+
+(defun igc--on-idle ()
+  (let* ((available-time (igc--predict-idle-time))
+         (interval igc--step-interval)
+         (multiplier (floor (/ available-time interval))))
+    (named-let step ((n multiplier))
+      (let* ((work-to-do (igc--arena-step interval n)))
+        (when (and work-to-do
+                   (> n 0)
+                   (not (accept-process-output nil 0)))
+          (step (1- n)))))))
+
 (provide 'igc)
 
 ;;; igc.el ends here.
diff --git a/src/igc.c b/src/igc.c
index 7a3961bc7cf..55fd133b954 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -4038,33 +4038,6 @@ buffer_it_next (struct igc_buffer_it *it)
   it->buf = Qnil;
 }
 
-static bool
-arena_step (void)
-{
-  /* mps_arena_step does not guarantee to return swiftly.  And it seems
-     that it sometimes does an opportunistic full collection alleging
-     the client predicted lots of idle time.  But it doesn't tell how
-     it comes to that conclusion. This is caused by bug#79346 in MPS. */
-  if (!FIXNUMP (Vigc_step_interval)
-      || XFIXNUM (Vigc_step_interval) != 0)
-    {
-      double interval = 0;
-      if (NUMBERP (Vigc_step_interval))
-	{
-	  interval = XFLOATINT (Vigc_step_interval);
-	  if (interval < 0)
-	    interval = 0.05;
-	}
-
-      /* 1.0 is the lowest possible value for the third argument to
-	 mps_arena_step (bug#76505).  */
-      if (mps_arena_step (global_igc->arena, interval, 1.0))
-	return true;
-    }
-
-  return false;
-}
-
 static bool
 buffer_step (struct igc_buffer_it *it)
 {
@@ -4118,7 +4091,6 @@ igc_on_idle (void)
     bool work_done = false;
     work_done |= process_one_message (global_igc);
     work_done |= buffer_step (&buffer_it);
-    work_done |= arena_step ();
 
     /* Don't always exhaust the max time we want to spend here.  */
     if (!work_done)
@@ -5429,6 +5401,34 @@ DEFUN ("igc--remove-extra-dependency", Figc__remove_extra_dependency,
   return Qt;
 }
 
+DEFUN ("igc--arena-step", Figc__arena_step, Sigc__arena_step, 2, 2, 0,
+       doc: /* Do some GC work.
+
+INTERVAL is the time, in seconds, that MPS is permitted to take.
+
+MULTIPLIER is the number of further similar calls that the client
+program expects to make during this idle period.
+
+Return t if there was work to do, nil otherwise. */)
+  (Lisp_Object interval, Lisp_Object multiplier)
+{
+  /* mps_arena_step does not guarantee to return swiftly.  And it seems
+     that it sometimes does an opportunistic full collection alleging
+     the client predicted lots of idle time.  But it doesn't tell how
+     it comes to that conclusion. This is caused by bug#79346 in MPS. */
+
+  /* 1.0 is the lowest possible value for the multiplier argument to
+     mps_arena_step (bug#76505).  */
+
+  double secs = extract_float (interval);
+  if (secs < 0.0)
+    xsignal1 (Qrange_error, interval);
+  CHECK_FIXNAT (multiplier);
+  EMACS_INT n = XFIXNAT (multiplier);
+  bool work_to_do = mps_arena_step (global_igc->arena, secs, n);
+  return work_to_do ? Qt : Qnil;
+}
+
 /***********************************************************************
 				  Init
  ***********************************************************************/
@@ -5452,18 +5452,11 @@ syms_of_igc (void)
   defsubr (&Sigc__set_commit_limit);
   defsubr (&Sigc__add_extra_dependency);
   defsubr (&Sigc__remove_extra_dependency);
+  defsubr (&Sigc__arena_step);
   DEFSYM (Qambig, "ambig");
   DEFSYM (Qexact, "exact");
   Fprovide (intern_c_string ("mps"), Qnil);
 
-  DEFVAR_LISP ("igc-step-interval", Vigc_step_interval,
-    doc: /* How much time MPS is allowed to spend in GC when Emacs is idle.
-The value is in seconds, and should be a non-negative integer or float.
-The default value is 0 which means not to do anything when idle.
-Negative values and values that are not numbers are handled as if they
-were the default value.  */);
-  Vigc_step_interval = make_fixnum (0);
-
   DEFVAR_BOOL ("igc--balance-intervals", igc__balance_intervals,
      doc: /* Whether to balance buffer intervals when idle.  */);
   igc__balance_intervals = false;
-- 
2.47.2