Subject: Re: bug#79023: 30.1.90; Suspicion of memory leak on internal_redisplay (MacOS)

From: Alan Third <[email-masked]>
Date: Sun, 31 Aug 2025 20:46:03 +0100

On Mon, Aug 11, 2025 at 08:47:44PM +0200, Przemysław Alexander Kamiński wrote:
> 
> I'm attaching 6 patches (for easy review/scrutiny/integration).
> Those patches that I marked as tested was used by me for >1 week.
> 
> * render_block_small.patch (significant impact, tested):
> 
> This is similar patch to previous one, but it's minimal. It's crucial
> to avoid "blink" redraws.
> Still breaks smooth resizing, but frame-resize-pixelwise set to true
> makes it unnoticable.

I'm not convinced this is the way to do this. I still think a better
solution would be to move the redisplay call to the EmacsLayer's
'display' method. As I understand it the system should only call that
method when it actually wants to display the output. Is that called
too often?

> * cache_all_font_results.patch (significant impact, requires testing):
> 
> This one prevents huge leak. I was trying to debug emacs-mac version (which
> has very deep traces for allocations, more 256 deep, which is limit for
> Instrumentations).
> 
> That led to full malloc logging and discovery that macfont listings were
> allocating plenty of font-related data. Some of it it never cleared.
> 
> I didn't test it and worry that caching "null" result might introduce
> some bugs, but it was hundreds of megabytes of allocations on -Q instance.

I can't comment on this as I have no idea how macfont.m works. Perhaps
some of the people working on the Mac port can help.

> * dont_flush_cg.patch (medium impact, tested):
> 
> Based on documentation of CGContextFlush it should seldom be used, as
> it's much better to give OS possibility to decide when to flush. It
> prevents non-needed renders (and allocations etc.)

See notes below.

> * misc_releases.patch (small-mid impact, tested):
> 
> This one is based on Rudolf Adamkovič patch with some minor extra
> releases and adjustments
> (like releasing an object before setting it to nil ;))

EmacsLayer is only used on Cocoa, and only on certain versions, so
anything that references it needs to be limited. Look at the #if
before the definition of EmacsLayer.

> * ns_native_api_dict.patch (small-mid impact, needs testing):
> 
> A small leak, but modified function had ~600_000 allocations after very
> short test runs. Seems like it wasted a lot of power, so I decided it
> should be checking dictionary hydrated at start and queried later
> instead of checking during runtime.
> 
> I believe that high NSString allocation amount was indirectly caused by
> using autorelease pools.

LGTM except for the same problem as above. You'll need to look at the
#if or #ifdefs around the code and ensure the new code is correctly
excluded when building on GNUstep.

I think you also removed a #if but left the associated #else and
#endif, although I could be wrong about that.

> * nullify_frame.patch (small impact, tested):
> 
> releases reference to a frame, as I've found traces in which resources
> were left behind, small leak.

This one is funny as emacsframe is a pointer to a "struct frame" and
we're not using garbage collection in NS so I don't see how simply
having a reference in a released object can prevent that struct from
being properly freed. Am I missing something?

> +  /* Don't reuse inner - cleanup is robust enough */
> +  emacsframe = nil;
> +

And I don't understand the comment.

> diff --git a/src/nsterm.m b/src/nsterm.m
> index b006b4d5dd..057d876a35 100644
> --- a/src/nsterm.m
> +++ b/src/nsterm.m
> @@ -9033,7 +9033,6 @@
> 
>  #if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
>    CGContextRef context = [(EmacsLayer *)[self layer] getContext];
> -  CGContextFlush (context);
> 
>    double scale = [[self window] backingScaleFactor];
>    int bpp = CGBitmapContextGetBitsPerPixel (context) / 8;

Immediately after this flush we access the pixels directly in the
buffer, so we *require* that all drawing actions in the context have
been completed, otherwise we risk doing actions out of order.
Therefore this flush is required.

> @@ -10994,7 +10993,6 @@
>    if (!context)
>      return;
> 
> -  CGContextFlush (context);
>    CGContextRelease (context);
>    context = NULL;

This one is probably superfluous as the CGContextRelease will probably
do the flush itself, but I don't know that for sure and we definitely
need it flushed before we can perform the next action, so I don't see
this as a problem.

Aside from that I think there was maybe some non-GNU-standard style C
in the nsimage patch, but I'm not entirely clear how it should look
myself.

Thanks!
-- 
Alan Third