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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 78735 in the body.
You can then email your comments to 78735 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Helmut Eller <eller.helmut <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: feature/igc: [PATCH] Reduce the size of the kbd-buffer GC root
Date: Mon, 09 Jun 2025 21:36:45 +0200

[Message part 1 (text/plain, inline)]

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.

[0001-Reduce-the-size-of-the-kbd-buffer-GC-root.patch (text/x-diff, attachment)]

Message #8 received at 78735 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 78735 <at> debbugs.gnu.org
Subject: Re: bug#78735: feature/igc: [PATCH] Reduce the size of the kbd-buffer
 GC root
Date: Mon, 09 Jun 2025 20:08:36 +0000

"Helmut Eller" <eller.helmut <at> gmail.com> 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

Message #11 received at 78735 <at> debbugs.gnu.org (full text, mbox):

From: Helmut Eller <eller.helmut <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 78735 <at> debbugs.gnu.org
Subject: Re: bug#78735: feature/igc: [PATCH] Reduce the size of the
 kbd-buffer GC root
Date: Tue, 10 Jun 2025 08:46:51 +0200

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.

Helmut

Message #16 received at 78735-done <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 78735-done <at> debbugs.gnu.org
Subject: Re: bug#78735: feature/igc: [PATCH] Reduce the size of the kbd-buffer
 GC root
Date: Fri, 13 Jun 2025 13:07:50 +0000

"Helmut Eller" <eller.helmut <at> gmail.com> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.