Re: Memory leak in keyboard variables?

Re: Memory leak in keyboard variables?

[Kenichi Handa]

> One thing I noticed is the xftfont_open is run whenever we
> call "emacsclient -c", but xftfont_close is never called.
> It appears that font_open_entity is called to allocate a
> font object, it is in general not recorded anywhere.

No.  A font object is recorded in FONT_OBJLIST_INDEX of a
font-entity.

> Could this be part of the problem?

The strategy is to record all font-objects in font-entities, and
record all font-entities in a cache of each font-backend.  The caches
are freed when `delete-frame' calls font_update_drivers with
new_drivers as nil through this calling sequence.

font_update_drivers -> font_finish_cache -> font_clear_cache

At that time all font-entities and font-obects are freed.

If there's a memory leak for font objectes, it means that there is a
bug at some code implementing the above strategy.

I don't have a time to tackle this problem at the moment,
but I'll take care of it as soon as possible.

---
Kenichi Handa
handa@m17n.org

Re: Memory leak in keyboard variables?

[Chong Yidong]

Kenichi Handa <handa@m17n.org> writes:

> The strategy is to record all font-objects in font-entities, and
> record all font-entities in a cache of each font-backend.  The caches
> are freed when `delete-frame' calls font_update_drivers with
> new_drivers as nil

Thanks for the explanation.  It was very helpful.

I think the problem is that font_clear_cache is incorrectly written.
For some reason, it assumes that the font cache entries have the form

  (font-spec [entity1 entity2...])

when in fact, they have the form

  (font-spec entity1 entity2...)

The following patch to font_clear_cache frees 60-70k of memory per
terminal.

Do you know why font_clear_cache was written this way, and whether there
could be any other places in the font code that make this incorrect
assumption?

*** trunk/src/font.c.~1.99.~	2008-12-13 10:39:30.000000000 -0500
--- trunk/src/font.c	2008-12-14 22:06:26.000000000 -0500
***************
*** 2651,2671 ****
       struct font_driver *driver;
  {
    Lisp_Object tail, elt;
  
    /* CACHE = (DRIVER-TYPE NUM-FRAMES FONT-CACHE-DATA ...) */
    for (tail = XCDR (XCDR (cache)); CONSP (tail); tail = XCDR (tail))
      {
        elt = XCAR (tail);
!       if (CONSP (elt) && FONT_SPEC_P (XCAR (elt)) && VECTORP (XCDR (elt)))
  	{
! 	  Lisp_Object vec = XCDR (elt);
! 	  int i;
! 
! 	  for (i = 0; i < ASIZE (vec); i++)
  	    {
! 	      Lisp_Object entity = AREF (vec, i);
  
! 	      if (EQ (driver->type, AREF (entity, FONT_TYPE_INDEX)))
  		{
  		  Lisp_Object objlist = AREF (entity, FONT_OBJLIST_INDEX);
  
--- 2651,2671 ----
       struct font_driver *driver;
  {
    Lisp_Object tail, elt;
+   Lisp_Object tail2, entity;
  
    /* CACHE = (DRIVER-TYPE NUM-FRAMES FONT-CACHE-DATA ...) */
    for (tail = XCDR (XCDR (cache)); CONSP (tail); tail = XCDR (tail))
      {
        elt = XCAR (tail);
!       /* elt should have the form (FONT-SPEC FONT-ENTITY ...) */
!       if (CONSP (elt) && FONT_SPEC_P (XCAR (elt)))
  	{
! 	  for (tail2 = XCDR (elt); CONSP (tail2); tail2 = XCDR (tail2))
  	    {
! 	      entity = XCAR (tail2);
  
! 	      if (FONT_ENTITY_P (entity)
! 		  && EQ (driver->type, AREF (entity, FONT_TYPE_INDEX)))
  		{
  		  Lisp_Object objlist = AREF (entity, FONT_OBJLIST_INDEX);
  

image cache (was: Memory leak in keyboard variables?)

[Stefan Monnier]

BTW, does anyone know why each frame has an image cache (even though
there's really only one image cache per terminal)?  It seems that code
could be simplified/improved if the image cache pointers were moved from
the frame objects to the terminal objects.


        Stefan

Re: image cache (was: Memory leak in keyboard variables?)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sun, 14 Dec 2008 23:06:55 -0500
> Cc: emacs-devel@gnu.org, Kenichi Handa <handa@m17n.org>
> 
> 
> BTW, does anyone know why each frame has an image cache (even though
> there's really only one image cache per terminal)?

A left-over from before mult-tty?

Re: image cache

[Stefan Monnier]

>> BTW, does anyone know why each frame has an image cache (even though
>> there's really only one image cache per terminal)?
> A left-over from before multi-tty?

I don't think it's related.  The same "problem" was there in Emacs-22,
and Emacs-21.


        Stefan

Re: image cache

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> BTW, does anyone know why each frame has an image cache (even though
>>> there's really only one image cache per terminal)?
>> A left-over from before multi-tty?
>
> I don't think it's related.  The same "problem" was there in Emacs-22,
> and Emacs-21.

Note that fixing requires changing all the functions in image.c that
require a frame argument, so that they use a terminal argument instead.
Otherwise, if you associate image caches to terminals, you run into
problems with terminals that have no frame (e.g. terminals created using
x-open-connection), because you can't supply those image functions with
a frame argument.

Re: image cache

[Stefan Monnier]

>>>> BTW, does anyone know why each frame has an image cache (even though
>>>> there's really only one image cache per terminal)?
>>> A left-over from before multi-tty?
>> I don't think it's related.  The same "problem" was there in Emacs-22,
>> and Emacs-21.

> Note that fixing requires changing all the functions in image.c that
> require a frame argument, so that they use a terminal argument instead.
> Otherwise, if you associate image caches to terminals, you run into
> problems with terminals that have no frame (e.g. terminals created using
> x-open-connection), because you can't supply those image functions with
> a frame argument.

Yes, such a change will/would imply a whole bunch of changes, indeed.
I'm just wondering what is the reason for having chosen frames rather
than terminals when writing the code.  Seeing the current state of the
code, it appears that terminals would have been a much more natural
choice, so either I'm missing something, or something has changed which
makes it the natural choice now even tho it wasn't earlier, or the
author had some odd notion of "natural" (or maybe didn't even think
much about it and just chose it arbitrarily).


        Stefan

Re: image cache

[Miles Bader]

Chong Yidong <cyd@stupidchicken.com> writes:
> Note that fixing requires changing all the functions in image.c that
> require a frame argument, so that they use a terminal argument instead.
> Otherwise, if you associate image caches to terminals, you run into
> problems with terminals that have no frame (e.g. terminals created using
> x-open-connection), because you can't supply those image functions with
> a frame argument.

Frameless terminals can't use those functions currently anyway, right?

So one could change the current implementation to put the cache in the
terminal data structure, but preserve the current interfaces.  This
would yield more sharing, without losing functionality.

-Miles

-- 
You can hack anything you want, with TECO and DDT.

Re: image cache

[Chetan Pandya]

--- On Tue, 12/16/08, Miles Bader <miles@gnu.org> wrote:


> Chong Yidong <cyd@stupidchicken.com> writes:
> > Note that fixing requires changing all the functions in image.c that
> > require a frame argument, so that they use a terminal argument instead.
> > Otherwise, if you associate image caches to terminals, you run into
> > problems with terminals that have no frame (e.g. terminals created using
> > x-open-connection), because you can't supply those image functions with
> > a frame argument.
> 
> Frameless terminals can't use those functions currently
> anyway, right?
> 
> So one could change the current implementation to put the
> cache in the
> terminal data structure, but preserve the current
> interfaces.  This
> would yield more sharing, without losing functionality.

Isn't that how it is currently?

From frame.h:
/* Return a pointer to the image cache of frame F.  */
#define FRAME_IMAGE_CACHE(F) ((F)->terminal->image_cache)

Chetan
> 
> -Miles
> 
> -- 
> You can hack anything you want, with TECO and DDT.

Re: image cache

[Chetan Pandya]

I looked at this some more and looks like it is not so everywhere. However, there seem to be more problems. It isn't clear how this is organized. For examples images are shared across frames, but there is no reference counting to know when it can be refreshed without affecting other frames and when it cannot. 


--- On Tue, 12/16/08, Chetan Pandya <pandyacus@sbcglobal.net> wrote:

> From: Chetan Pandya <pandyacus@sbcglobal.net>
> Subject: Re: image cache
> To: "Chong Yidong" <cyd@stupidchicken.com>, "Miles Bader" <miles@gnu.org>
> Cc: "Eli Zaretskii" <eliz@gnu.org>, handa@m17n.org, "Stefan Monnier" <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> Date: Tuesday, December 16, 2008, 4:56 AM
> --- On Tue, 12/16/08, Miles Bader <miles@gnu.org>
> wrote:
> 
> 
> > Chong Yidong <cyd@stupidchicken.com> writes:
> > > Note that fixing requires changing all the
> functions in image.c that
> > > require a frame argument, so that they use a
> terminal argument instead.
> > > Otherwise, if you associate image caches to
> terminals, you run into
> > > problems with terminals that have no frame (e.g.
> terminals created using
> > > x-open-connection), because you can't supply
> those image functions with
> > > a frame argument.
> > 
> > Frameless terminals can't use those functions
> currently
> > anyway, right?
> > 
> > So one could change the current implementation to put
> the
> > cache in the
> > terminal data structure, but preserve the current
> > interfaces.  This
> > would yield more sharing, without losing
> functionality.
> 
> Isn't that how it is currently?
> 
> From frame.h:
> /* Return a pointer to the image cache of frame F.  */
> #define FRAME_IMAGE_CACHE(F)
> ((F)->terminal->image_cache)
> 
> Chetan
> > 
> > -Miles
> > 
> > -- 
> > You can hack anything you want, with TECO and DDT.

Re: Memory leak in keyboard variables?

[Kenichi Handa]

In article <87oczeuo0a.fsf@cyd.mit.edu>, Chong Yidong <cyd@stupidchicken.com> writes:

> I think the problem is that font_clear_cache is incorrectly written.
> For some reason, it assumes that the font cache entries have the form

>   (font-spec [entity1 entity2...])

> when in fact, they have the form

>   (font-spec entity1 entity2...)

> The following patch to font_clear_cache frees 60-70k of memory per
> terminal.

Ah, it seems that your patch is correct.

> Do you know why font_clear_cache was written this way,

In the early version of font-backend codes,
font_driver->list returns a vector.  I changed it to return
a list at sometime, but have forgotten to adjust
font_clear_cache.

> and whether there
> could be any other places in the font code that make this incorrect
> assumption?

I've just read through font.c, but the other places look ok.

---
Kenichi Handa
handa@m17n.org