Subject: bug#78735: closed (Re: bug#78735: feature/igc: [PATCH] Reduce the size of the kbd-buffer GC root)

From: [email-masked] (GNU bug Tracking System)
Date: Fri, 13 Jun 2025 13:09:02 +0000

Your bug report

#78735: feature/igc: [PATCH] Reduce the size of the kbd-buffer GC root

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
78735: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=78735
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#78735: feature/igc: [PATCH] Reduce the size of the kbd-buffer GC root
From: Pip Cet <[email-masked]>
To: Helmut Eller <[email-masked]>
Cc: [email-masked]
Date: Fri, 13 Jun 2025 13:07:50 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

"Helmut Eller" <[email-masked]> writes:

> On Mon, Jun 09 2025, Pip Cet wrote:
>
> [...]
>>> +static union buffered_input_event *
>>> +prev_kbd_event (union buffered_input_event *kbd_buffer,
>>> +		union buffered_input_event *ptr)
>>> +{
>>> +  return ptr == kbd_buffer ? kbd_buffer + KBD_BUFFER_SIZE - 1 : ptr - 1;
>>> +}
>>> +
>>> +static union buffered_input_event *
>>> +next_kbd_event (union buffered_input_event *kbd_buffer,
>>> +		union buffered_input_event *ptr)
>>> +{
>>> +  return ptr == kbd_buffer + KBD_BUFFER_SIZE - 1 ? kbd_buffer : ptr + 1;
>>> +}
>>
>> Just out of curiosity, is there a reason for the extra kbd_buffer
>> argument?  Since this code is replicated a few times, maybe it would be
>> better to keep next_kbd_event in keyboard.c identical to the one in
>> igc.c.  Maybe not, though...
>
> The extra argument avoids accessing the global variable; that's all.
>
>>> +  union buffered_input_event *fetch
>>> +    = prev_kbd_event (kbd_buffer, kbd_fetch_ptr);
>>> +  union buffered_input_event *store
>>> +    = next_kbd_event (kbd_buffer, kbd_store_ptr);
>>> +
>>> +  if (fetch < store)
>>> +    return scan_ambig (ss, fetch, store, closure);
>>
>> I think this will fail if kbd_store_ptr + 1 == kbd_fetch_ptr (i.e. the
>> kbd_buffer is full)?
>
> Hmm, indeed.
>
>> if (fetch < store - 1)
>>
>> should work, though, and it's not like this case is common enough to
>> worry about scanning the same event twice.
>
> Yes, that should work.

I've applied a new version of this patch, and am closing the bug.
Thanks again, Helmut!

Pip

Subject: feature/igc: [PATCH] Reduce the size of the kbd-buffer GC root
From: Helmut Eller <[email-masked]>
To: [email-masked]
Date: Mon, 09 Jun 2025 21:36:45 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

This is a proposal to reduce of the size of the kbd-buffer root.
Currently the kdb-buffer is an area of ~250kb that is scanned
ambiguously.

With the patch, instead of tracing the entire kbd_buffer, we only scan
the region from kbd_fetch_ptr - 1 to kbd_store_ptr + 1.  The -1/+1 is
supposed to cover the cases where MPS stops the mutator while those
pointers are being updated.  If the kbd_buffer is empty, then only 180
bytes are scanned.  It's still scanned ambiguously.

>From d1e9eeb7d3cfea48d1037564fda50b511e80a52b Mon Sep 17 00:00:00 2001
From: Helmut Eller <[email-masked]>
Date: Mon, 9 Jun 2025 18:28:10 +0200
Subject: [PATCH] Reduce the size of the kbd-buffer GC root

* src/igc.c (root_create_kbd_buffer): New.
(root_create_main_thread): Use it.
(scan_kbd_buffer_ambig): New. The actual scan function.
(next_kbd_event, prev_kbd_event): New helpers, similar to next_kbd_event
in keyboard.c.
* src/keyboard.c (init_keyboard): Move the GC root creation code to
root_create_kbd_buffer in igc.c.
---
 src/igc.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/keyboard.c |  4 ----
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/src/igc.c b/src/igc.c
index f63b0fe8c88..5f7a1a78cda 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -60,6 +60,7 @@
 #include "thread.h"
 #include "treesit.h"
 #include "termchar.h"
+#include "keyboard.h"
 #ifdef HAVE_WINDOW_SYSTEM
 #include TERM_HEADER
 #endif /* HAVE_WINDOW_SYSTEM */
@@ -1725,6 +1726,48 @@ scan_tty_list (mps_ss_t ss, void *start, void *end, void *closure)
   return MPS_RES_OK;
 }
 
+static union buffered_input_event *
+prev_kbd_event (union buffered_input_event *kbd_buffer,
+		union buffered_input_event *ptr)
+{
+  return ptr == kbd_buffer ? kbd_buffer + KBD_BUFFER_SIZE - 1 : ptr - 1;
+}
+
+static union buffered_input_event *
+next_kbd_event (union buffered_input_event *kbd_buffer,
+		union buffered_input_event *ptr)
+{
+  return ptr == kbd_buffer + KBD_BUFFER_SIZE - 1 ? kbd_buffer : ptr + 1;
+}
+
+static mps_res_t
+scan_kbd_buffer_ambig (mps_ss_t ss, void *start, void *end, void *closure)
+{
+  igc_assert (start == kbd_buffer);
+  igc_assert (end == kbd_buffer + ARRAYELTS (kbd_buffer));
+
+  /* Instead of tracing the entire kbd_buffer, only scan the region from
+     kbd_fetch_ptr - 1 to kbd_store_ptr + 1.  The -1/+1 is supposed to
+     cover the cases where MPS stops the mutator while those pointers
+     are being updated. */
+
+  union buffered_input_event *fetch
+    = prev_kbd_event (kbd_buffer, kbd_fetch_ptr);
+  union buffered_input_event *store
+    = next_kbd_event (kbd_buffer, kbd_store_ptr);
+
+  if (fetch < store)
+    return scan_ambig (ss, fetch, store, closure);
+  else
+    {
+      mps_res_t res
+	= scan_ambig (ss, fetch, kbd_buffer + KBD_BUFFER_SIZE, closure);
+      if (res == MPS_RES_OK)
+	res = scan_ambig (ss, kbd_buffer, store, closure);
+      return res;
+    }
+}
+
 /***********************************************************************
 			 Default pad, fwd, ...
  ***********************************************************************/
@@ -2905,6 +2948,14 @@ root_create_tty_list (struct igc *gc)
 		     scan_tty_list, "tty-list");
 }
 
+static void
+root_create_kbd_buffer (struct igc *gc)
+{
+  root_create (gc, kbd_buffer, kbd_buffer + ARRAYELTS (kbd_buffer),
+	       mps_rank_ambig (), scan_kbd_buffer_ambig, NULL,
+	       true, "kbd-buffer");
+}
+
 static void
 root_create_main_thread (struct igc *gc)
 {
@@ -4934,6 +4985,7 @@ make_igc (void)
   root_create_main_thread (gc);
   root_create_exact_ptr (gc, &current_thread);
   root_create_exact_ptr (gc, &all_threads);
+  root_create_kbd_buffer (gc);
 
   enable_messages (gc, true);
   return gc;
diff --git a/src/keyboard.c b/src/keyboard.c
index 72c6e4c17d8..1c2b4ba9e63 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -12892,10 +12892,6 @@ delete_kboard (KBOARD *kb)
 void
 init_keyboard (void)
 {
-#ifdef HAVE_MPS
-  igc_root_create_ambig (kbd_buffer, kbd_buffer + ARRAYELTS (kbd_buffer),
-			 "kbd-buffer");
-#endif
   /* This is correct before outermost invocation of the editor loop.  */
   command_loop_level = -1;
   quit_char = Ctl ('g');
-- 
2.39.5