GNU bug report logs - #79771
[PATCH] [NS] Throttled cache for window handlers

Previous Next

To reply to this bug, email your comments to 79771 AT debbugs.gnu.org.

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

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

From: Przemysław Alexander Kamiński
 <alexander <at> kaminski.se>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] [NS] Throttled cache for window handlers
Date: Wed, 05 Nov 2025 19:02:40 +0100

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

Attached you can find throttled cache for window handler. It's not yet
ready, but I'd like to present it nonetheless.

Problem this patch solves is a prevention of zombie window handler
generation. During window creation handler is duplicated (most likely
because of control passing mechanism on NS) which not only produces
slowdowns process down but also leaves memory artifacts behind (as
previous handler is simply forfeited).

This patch is stable in my eyes I've been running it for >2 months
without any issues.

-- 
Przemysław Alexander Kamiński (vel xlii vel exlee)
https://xlii.space  ||  https://codeberg.org/exlee

[throttled_cache_0000.patch (text/x-patch, attachment)]

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

From: Robert Pluim <rpluim <at> gmail.com>
To: Przemysław Alexander Kamiński
 <alexander <at> kaminski.se>
Cc: 79771 <at> debbugs.gnu.org
Subject: Re: bug#79771: [PATCH] [NS] Throttled cache for window handlers
Date: Wed, 05 Nov 2025 19:46:05 +0100

>>>>> On Wed, 05 Nov 2025 19:02:40 +0100, Przemysław Alexander Kamiński <alexander <at> kaminski.se> said:

    Alexander> Attached you can find throttled cache for window handler. It's not yet
    Alexander> ready, but I'd like to present it nonetheless.

    Alexander> Problem this patch solves is a prevention of zombie window handler
    Alexander> generation. During window creation handler is duplicated (most likely
    Alexander> because of control passing mechanism on NS) which not only produces
    Alexander> slowdowns process down but also leaves memory artifacts behind (as
    Alexander> previous handler is simply forfeited).

    Alexander> This patch is stable in my eyes I've been running it for >2 months
    Alexander> without any issues.

    Alexander> -- 
    Alexander> Przemysław Alexander Kamiński (vel xlii vel exlee)
    Alexander> https://xlii.space  ||  https://codeberg.org/exlee

    Alexander> diff --git a/configure.ac b/configure.ac
    Alexander> index ccc8719c09..a10176e812 100644
    Alexander> --- a/configure.ac
    Alexander> +++ b/configure.ac
    Alexander> @@ -2931,7 +2931,7 @@
    Alexander>       test "$locallisppathset" = no && locallisppath="\${ns_appresdir}/site-lisp"
    Alexander>       INSTALL_ARCH_INDEP_EXTRA=
    Alexander>    fi
    Alexander> -
    Alexander> +  NS_OBJ="optimization.o"
    Alexander>    NS_OBJC_OBJ="nsterm.o nsfns.o nsmenu.o nsselect.o nsimage.o $ns_fontfile"
    Alexander>  fi

Iʼm assuming this is only going to be for NS, in which case the
filename should start with "ns".

    Alexander>  CFLAGS="$tmp_CFLAGS"
    Alexander> diff --git a/src/Makefile.in b/src/Makefile.in
    Alexander> index a2f7ea011c..4e4c787e7b 100644
    Alexander> --- a/src/Makefile.in
    Alexander> +++ b/src/Makefile.in
    Alexander> @@ -477,6 +477,7 @@
    Alexander>  SOME_MACHINE_OBJECTS = dosfns.o msdos.o \
    Alexander>    xterm.o xfns.o xmenu.o xselect.o xrdb.o xsmfns.o fringe.o image.o \
    Alexander>    fontset.o dbusbind.o cygw32.o \
    Alexander> +  optimization.o \
    Alexander>    nsterm.o nsfns.o nsmenu.o nsselect.o nsimage.o nsfont.o macfont.o \
    Alexander>    nsxwidget.o \
    Alexander>    w32.o w32console.o w32cygwinx.o w32fns.o w32heap.o w32inevt.o w32notify.o \
    Alexander> diff --git a/src/deps.mk b/src/deps.mk
    Alexander> index e2c786f8fb..0a27bc5c63 100644
    Alexander> --- a/src/deps.mk
    Alexander> +++ b/src/deps.mk
    Alexander> @@ -161,6 +161,7 @@
    Alexander>  nsterm.o: nsterm.m blockinput.h atimer.h systime.h syssignal.h nsterm.h \
    Alexander>     nsgui.h frame.h charset.h ccl.h dispextern.h fontset.h termhooks.h \
    Alexander>     termopts.h termchar.h disptab.h buffer.h window.h keyboard.h \
    Alexander> +   optimization.h \
    Alexander>     $(INTERVALS_H) process.h coding.h lisp.h $(config_h)
    Alexander>  nsselect.o: nsselect.m blockinput.h nsterm.h nsgui.h frame.h lisp.h $(config_h)
    Alexander>  process.o: process.c process.h buffer.h window.h termhooks.h termopts.h \
    Alexander> diff --git a/src/nsterm.m b/src/nsterm.m
    Alexander> index 74ee1219bb..b6a585cb30 100644
    Alexander> --- a/src/nsterm.m
    Alexander> +++ b/src/nsterm.m
    Alexander> @@ -71,6 +71,7 @@
    Alexander>  #include "macfont.h"
    Alexander>  #include <Carbon/Carbon.h>
    Alexander>  #include <IOSurface/IOSurface.h>
    Alexander> +#include "optimization.h"
    Alexander>  #endif
 
    Alexander>  static EmacsMenu *dockMenu;
    Alexander> @@ -9456,13 +9457,23 @@
    Alexander>  }
 
 
    Alexander> +
    Alexander> +static tc_handler window_cache = {.result = nil, .expiration = {0,0}, .hold = {0,1000000}};
    Alexander> +
    Alexander>  - (instancetype) initWithEmacsFrame: (struct frame *) f
    Alexander>                           fullscreen: (BOOL) fullscreen
    Alexander>                               screen: (NSScreen *) screen
    Alexander>  {
    Alexander> +  EmacsWindow **cached;
    Alexander> +  if (throttle_cache_get (&window_cache, *cached) == TC_OK)
    Alexander> +    {
    Alexander> +      [*cached retain];
    Alexander> +      return *cached;
    Alexander> +    };
    Alexander>    NSWindowStyleMask styleMask;
    Alexander>    int width, height;
 
    Alexander> +
    Alexander>    NSTRACE ("[EmacsWindow initWithEmacsFrame:fullscreen:screen:]");
 
    Alexander>    if (fullscreen)
    Alexander> @@ -9564,13 +9575,13 @@
    Alexander>          [self setTabbingMode:NSWindowTabbingModeDisallowed];
    Alexander>  #endif
    Alexander>      }
    Alexander> -
    Alexander> +  throttle_cache_set (&window_cache, &self);
    Alexander>    return self;
    Alexander>  }
 
    Alexander> -
    Alexander>  - (void)createToolbar: (struct frame *)f
    Alexander>  {
    Alexander> +
    Alexander>    if (FRAME_UNDECORATED (f)
    Alexander>        || [self styleMask] == NSWindowStyleMaskBorderless
    Alexander>        || !FRAME_EXTERNAL_TOOL_BAR (f)
    Alexander> diff --git a/src/optimization.c b/src/optimization.c
    Alexander> new file mode 100644
    Alexander> index 0000000000..37bd77cd16
    Alexander> --- /dev/null
    Alexander> +++ b/src/optimization.c
    Alexander> @@ -0,0 +1,68 @@
    Alexander> +#include "config.h"
    Alexander> +#include <pthread.h>
    Alexander> +

Is "pthread.h" really required?

    Alexander> +#include <stdlib.h>
    Alexander> +#include <string.h>
    Alexander> +#include "optimization.h"
    Alexander> +
    Alexander> +/* /\* Prototypes *\/ */
    Alexander> +/* bool throttle_cache_expired (tc_handler *); */
    Alexander> +/* tc_error throttle_cache_handler_create (tc_handler *, */
    Alexander> +/* 					struct timespec); */
    Alexander> +
    Alexander> +/* tc_error throttle_cache_handler_get (tc_handler *, void *); */
    Alexander> +/* bool throttle_cache_expired (tc_handler *); */
    Alexander> +/* tc_error throttle_cache_set (tc_handler *, void *); */
    Alexander> +
    Alexander> +#define TC_HANDLER_INITIALIZER(SECS, NSECS)     \
    Alexander> +   { .result = NULL,	\
    Alexander> +      .expiration = { 0, 0 }, \
    Alexander> +       .hold = { SECS,NSECS } };

This is not used.

    Alexander> +tc_error
    Alexander> +throttle_cache_handler_create (tc_handler *ptr, struct timespec hold)
    Alexander> +{
    Alexander> +  if (ptr == NULL)
    Alexander> +    return TC_CREATE_NULL_PTR;
    Alexander> +
    Alexander> +  *ptr = (tc_handler){ .hold = hold,
    Alexander> +		       .expiration = { 0, 0 },
    Alexander> +		       .result = NULL };
    Alexander> +
    Alexander> +  return OK;
    Alexander> +}
    Alexander> +
    Alexander> +tc_error throttle_cache_get (tc_handler *ptr, void *result)
    Alexander> +{
    Alexander> +  if (throttle_cache_expired (ptr))
    Alexander> +    {
    Alexander> +      return TC_RESULT_EXPIRED;
    Alexander> +    }
    Alexander> +
    Alexander> +  if (ptr->result == NULL)
    Alexander> +    return TC_RESULT_NULL;
    Alexander> +

For 'if' followed by a single line we prefer to omit the braces, like
youʼve done here, but not 2 lines up 😀

    Alexander> +  result = ptr->result;
    Alexander> +  return OK;
    Alexander> +}
    Alexander> +tc_error
    Alexander> +throttle_cache_set (tc_handler *ptr, void *result)
    Alexander> +{
    Alexander> +  if(result == NULL) return TC_SET_NULL_PTR;

Space after 'if'

    Alexander> +  clock_gettime (CLOCK_MONOTONIC, &ptr->expiration);
    Alexander> +  ptr->expiration.tv_sec += ptr->hold.tv_sec;
    Alexander> +  ptr->expiration.tv_nsec += ptr->hold.tv_nsec;
    Alexander> +  ptr->result = result;
    Alexander> +}
    Alexander> +
    Alexander> +bool
    Alexander> +throttle_cache_expired (tc_handler *ptr)
    Alexander> +{
    Alexander> +  struct timespec ts;
    Alexander> +  clock_gettime (CLOCK_MONOTONIC, &ts);
    Alexander> +  if (ts.tv_sec > ptr->expiration.tv_sec)
    Alexander> +    return true;
    Alexander> +  if (ts.tv_nsec > ptr->expiration.tv_nsec)
    Alexander> +    return true;
    Alexander> +
    Alexander> +  return false;
    Alexander> +}

Since I doubt you need extreme accuracy, you could use
`monotonic_coarse_timespec' instead of `clock_gettime'. See also
`timespec_cmp'.

As a general comment, if your helper functions are only used in this
file, please make them static.

    Alexander> diff --git a/src/optimization.h b/src/optimization.h
    Alexander> new file mode 100644
    Alexander> index 0000000000..eee7dcb376
    Alexander> --- /dev/null
    Alexander> +++ b/src/optimization.h
    Alexander> @@ -0,0 +1,35 @@
    Alexander> +#ifndef OPTIMIZATION_H
    Alexander> +#define OPTIMIZATION_H
    Alexander> +#include <time.h>
    Alexander> +
    Alexander> +#define TC_HANDLER_INIT_OBJC(SEC,NSEC)  {.result = nil, .expiration = {0,0}, .hold = {SEC,NSEC}}
    Alexander> +#define TC_HANDLER_INIT(SEC,NSEC)  {.result = NULL, .expiration = {0,0}, .hold = {SEC,NSEC}}
    Alexander> +

These are not used.

    Alexander> +#define TC_MAX_HOLD_SECONDS 60

Nor is this.

    Alexander> +
    Alexander> +typedef enum tc_error
    Alexander> +{
    Alexander> +  TC_OK = 0,
    Alexander> +  OK = 0,

Iʼd prefer it if all the enum values started with TC_

    Alexander> +  TC_CREATE_NULL_PTR = 1,
    Alexander> +  TC_SET_NULL_PTR = 1,

Robertʼs second rule of debuggability is that all your returned
constants shall be distinct. In other places we use

    1<<0,
    1<<1,

etc, which might be preferable.

    Alexander> +  TC_RESULT_EXPIRED = 2,
    Alexander> +  TC_RESULT_NULL = 4,
    Alexander> +} tc_error;
    Alexander> +
    Alexander> +typedef struct tc_handler
    Alexander> +{
    Alexander> +  struct timespec expiration;
    Alexander> +  struct timespec hold;
    Alexander> +  void *result;
    Alexander> +} tc_handler;
    Alexander> +
    Alexander> +bool throttle_cache_expired (tc_handler *);
    Alexander> +tc_error throttle_cache_handler_create (tc_handler *,
    Alexander> +					struct timespec);
    Alexander> +
    Alexander> +tc_error throttle_cache_get (tc_handler *, void *);
    Alexander> +bool throttle_cache_expired (tc_handler *);
    Alexander> +tc_error throttle_cache_set (tc_handler *, void *);
    Alexander> +
    Alexander> +#endif /*OPTIMIZATION_H*/

Maybe itʼs just late, but I donʼt see how anything ever gets evicted
from the cache?

Robert
-- 

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: alexander <at> kaminski.se, 79771 <at> debbugs.gnu.org
Subject: Re: bug#79771: [PATCH] [NS] Throttled cache for window handlers
Date: Wed, 05 Nov 2025 21:38:01 +0200

> Cc: 79771 <at> debbugs.gnu.org
> From: Robert Pluim <rpluim <at> gmail.com>
> Date: Wed, 05 Nov 2025 19:46:05 +0100
> 
> Iʼm assuming this is only going to be for NS, in which case the
> filename should start with "ns".

And perhaps we should for now keep it specific to its one user, not a
separate file with something that is meant to be a general
infrastructure.  I'm not sure I see a place or a need for such a
general infrastructure in C.

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

From: Przemysław Alexander Kamiński
 <alexander <at> kaminski.se>
To: Eli Zaretskii <eliz <at> gnu.org>, Robert Pluim <rpluim <at> gmail.com>
Cc: 79771 <at> debbugs.gnu.org
Subject: Re: bug#79771: [PATCH] [NS] Throttled cache for window handlers
Date: Wed, 05 Nov 2025 21:09:59 +0100

-- 
On Wed, Nov 05, 2025 at 21:38:01 (+0200), Eli Zaretskii wrote:

>> Cc: 79771 <at> debbugs.gnu.org
>> From: Robert Pluim <rpluim <at> gmail.com>
>> Date: Wed, 05 Nov 2025 19:46:05 +0100
>> 
>> Iʼm assuming this is only going to be for NS, in which case the
>> filename should start with "ns".
>
> And perhaps we should for now keep it specific to its one user, not a
> separate file with something that is meant to be a general
> infrastructure.  I'm not sure I see a place or a need for such a
> general infrastructure in C.

Hi Robert,

The patch in question is just a proof of concept - I knew that wherever
I'd put it it'd be a highly probably - a miss, that's why I marked it as
0000 patch and isolated to separate file.

I disagree with a need of general infrastructure in C. While I'm
submitting NS patches now, I've also made attempts to optimize C code
and was surprised that no general optimization architecture exists like
specialized caches or memory allocation functions for things like arenas
or zones.

-- 
Przemysław Alexander Kamiński (vel xlii vel exlee)
https://xlii.space  ||  https://codeberg.org/exlee

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Przemysław Alexander Kamiński
 <alexander <at> kaminski.se>
Cc: rpluim <at> gmail.com, 79771 <at> debbugs.gnu.org
Subject: Re: bug#79771: [PATCH] [NS] Throttled cache for window handlers
Date: Wed, 05 Nov 2025 22:50:06 +0200

> From: Przemysław Alexander Kamiński
>  <alexander <at> kaminski.se>
> Cc: 79771 <at> debbugs.gnu.org
> Date: Wed, 05 Nov 2025 21:09:59 +0100
> 
> I disagree with a need of general infrastructure in C. While I'm
> submitting NS patches now, I've also made attempts to optimize C code
> and was surprised that no general optimization architecture exists like
> specialized caches or memory allocation functions for things like arenas
> or zones.

We introduce such infrastructure when it is needed, not because it
might be useful.  As long as there's just one user of this, it isn't
justified to generalize it, let alone give it a separate file.

Previous Next

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