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

From: Pip Cet <[email-masked]>
Date: Mon, 09 Jun 2025 20:08:36 +0000

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

> 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.

That sounds great to me.

> +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...

> +  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)?

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.

Pip