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

Image cache

[Juri Linkov]

After generating different images in the same file and visiting them after
every change in Emacs, I noticed that the displayed image doesn't
get updated.  Later I found the variable image-cache-eviction-delay and
changed its value to a small delay (1 sec), but it didn't have any effect
contrary to its documentation: the changed image was not updated in
the cache.  It seems this behavior is caused by the constant value
#define CLEAR_IMAGE_CACHE_COUNT 101 that checks the value of
image-cache-eviction-delay only on 101-st redisplay.  Surely, I can use
(add-hook 'post-command-hook 'clear-image-cache), but this is not
a general solution.

OTOH, I don't see why I should change the value of image-cache-eviction-delay,
if it is Emacs' job to update the cache when the image data changes.
So I found that updating the image cache after visiting an new image
was correct before the 2006-01-30 change in image-mode.el:

	* image-mode.el (image-toggle-display): Use file name if possible,
	instead of unnecessarily allocating a (possibly huge) lisp string.

But I don't blame this change.  It is a good optimization, but it
revealed a problem in cache processing code.  This code doesn't update
the cache for the file name when the file contents changes.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Image cache

[Chong Yidong]

Juri Linkov <juri@jurta.org> writes:

> After generating different images in the same file and visiting them after
> every change in Emacs, I noticed that the displayed image doesn't
> get updated... updating the image cache after visiting an new image
> was correct before the 2006-01-30 change in image-mode.el:
>
> 	* image-mode.el (image-toggle-display): Use file name if possible,
> 	instead of unnecessarily allocating a (possibly huge) lisp string.

Ugh.  Two simple solutions come to mind: (i) revert my change, or (ii)
clear the image cache on each call to image-toggle-display.  I don't
know which is worse, performance-wise -- any suggestions?

We could change the cache code for file image specs so that the file
modtime in included in the image hash.  But this can probably wait for
after the release -- for now, we can always call `clear-image-cache'
wherever changes to image files can be a problem.  (Aside from
image-mode, I don't know where else the image cache would be a
problem.)

Re: Image cache

[Mathias Dahl]

Chong Yidong <cyd@stupidchicken.com> writes:

> We could change the cache code for file image specs so that the file
> modtime in included in the image hash.  But this can probably wait for
> after the release -- for now, we can always call `clear-image-cache'
> wherever changes to image files can be a problem.  (Aside from
> image-mode, I don't know where else the image cache would be a
> problem.)

Tumme has the same problems and calls `clear-image-cache' in several
places, for example in `tumme-display-image'.

Re: Image cache

[Juri Linkov]

>> 	* image-mode.el (image-toggle-display): Use file name if possible,
>> 	instead of unnecessarily allocating a (possibly huge) lisp string.
>
> Ugh.  Two simple solutions come to mind: (i) revert my change, or (ii)
> clear the image cache on each call to image-toggle-display.  I don't
> know which is worse, performance-wise -- any suggestions?

Your change also has another undesirable effect: if doesn't handle the
situation when Emacs can't read the image.  It visits the image file in
text mode but thinks that it is in image mode (i.e. hides the cursor and
prints a wrong message in the echo area).  Before your change handling this
situation in image-mode.el was not nice either: with debug-on-error=t
it signalled an error "Cannot determine image type", and with
debug-on-error=nil this error was not reported at all.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Image cache

[Juri Linkov]

> Your change also has another undesirable effect: if doesn't handle the
> situation when Emacs can't read the image.  It visits the image file in
> text mode but thinks that it is in image mode (i.e. hides the cursor and
> prints a wrong message in the echo area).  Before your change handling this
> situation in image-mode.el was not nice either: with debug-on-error=t
> it signalled an error "Cannot determine image type", and with
> debug-on-error=nil this error was not reported at all.

This is due to the inability of create-image to determine the image type.
I noticed an unused function image-type-from-file-name in image.el that
looks like it was created specially for the function create-image.
I think the following patch does what was planned initially to do with
this function.  With this patch, JPEG files that `image-jpeg-p' fails to
recognize still get displayed in the image-mode's buffer correctly.

Index: lisp/image.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/image.el,v
retrieving revision 1.55
diff -c -r1.55 image.el
*** lisp/image.el	6 Feb 2006 14:33:34 -0000	1.55
--- lisp/image.el	14 Feb 2006 01:45:20 -0000
***************
*** 210,220 ****
    (cond ((null data-p)
  	 ;; FILE-OR-DATA is a file name.
  	 (unless (or type
! 		     (setq type (image-type-from-file-header file-or-data)))
! 	   (let ((extension (file-name-extension file-or-data)))
! 	     (unless extension
! 	       (error "Cannot determine image type"))
! 	     (setq type (intern extension)))))
  	(t
  	 ;; FILE-OR-DATA contains image data.
  	 (unless type
--- 223,231 ----
    (cond ((null data-p)
  	 ;; FILE-OR-DATA is a file name.
  	 (unless (or type
! 		     (setq type (image-type-from-file-header file-or-data))
! 		     (setq type (image-type-from-file-name file-or-data)))
! 	   (error "Cannot determine image type")))
  	(t
  	 ;; FILE-OR-DATA contains image data.
  	 (unless type

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Image cache

[Stefan Monnier]

> OTOH, I don't see why I should change the value of image-cache-eviction-delay,
> if it is Emacs' job to update the cache when the image data changes.
> So I found that updating the image cache after visiting an new image
> was correct before the 2006-01-30 change in image-mode.el:

> 	* image-mode.el (image-toggle-display): Use file name if possible,
> 	instead of unnecessarily allocating a (possibly huge) lisp string.

> But I don't blame this change.  It is a good optimization, but it
> revealed a problem in cache processing code.  This code doesn't update
> the cache for the file name when the file contents changes.

If someone has the time, it seems it migt be worth it to do something along
the following lines:

- add `loadtime' timestamp to the image data-structure so that staleness can
  be detected and a `refresh' function in the image_type data structure
  which, given the `loadtime' image `spec' says whether to reload or not.
- provide a new function (image-refresh IMAGE-SPEC).
- make create-image (used by image-toggle-display and others) use that
  new function.


        Stefan

Re: Image cache

[Richard M. Stallman]

    If someone has the time, it seems it migt be worth it to do something along
    the following lines:

    - add `loadtime' timestamp to the image data-structure so that staleness can
      be detected and a `refresh' function in the image_type data structure
      which, given the `loadtime' image `spec' says whether to reload or not.
    - provide a new function (image-refresh IMAGE-SPEC).
    - make create-image (used by image-toggle-display and others) use that
      new function.

Would someone please add that to etc/TODO?

Re: Image cache

[Juri Linkov]

>     If someone has the time, it seems it migt be worth it to do something along
>     the following lines:
>
>     - add `loadtime' timestamp to the image data-structure so that staleness can
>       be detected and a `refresh' function in the image_type data structure
>       which, given the `loadtime' image `spec' says whether to reload or not.
>     - provide a new function (image-refresh IMAGE-SPEC).
>     - make create-image (used by image-toggle-display and others) use that
>       new function.
>
> Would someone please add that to etc/TODO?

But it is a bug that the image cache doesn't get updated when the image file
content changes.  Is there an easy way to fix it?  Maybe to find all the
Emacs features that use images, and add `clear-image-cache'?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Image cache

[Chong Yidong]

Juri Linkov <juri@jurta.org> writes:

> But it is a bug that the image cache doesn't get updated when the image file
> content changes.  Is there an easy way to fix it?  Maybe to find all the
> Emacs features that use images, and add `clear-image-cache'?

I think most places it's not important to check for changes to image
files, e.g., the images in the toolbars.  I added a call to
clear-image-cache in image-mode.

Re: Image cache

[Mathias Dahl]

Juri Linkov <juri@jurta.org> writes:

> But it is a bug that the image cache doesn't get updated when the image file
> content changes.  Is there an easy way to fix it?  Maybe to find all the
> Emacs features that use images, and add `clear-image-cache'?

Wouldn't that be more of a workaround than a fix? What Stefan
suggested sounded more like a fix to me.

The reason I say this, is that when I added `clear-image-cache' to
tumme it felt like I worked around some "strange" behaviour in the
image cache, and at least back then I thought that eventually that
would be changed and I could remove the unnecessary function call.

Re: Image cache

[Miles Bader]

On 2/10/06, Juri Linkov <juri@jurta.org> wrote:
> But it is a bug that the image cache doesn't get updated when the image file
> content changes.  Is there an easy way to fix it?  Maybe to find all the
> Emacs features that use images, and add `clear-image-cache'?

That's not a good solution though, because it refreshes _every_ image;
if you use images heavily in emacs it can cause noticeable flickering
(e.g. I use large background images, so I definitely see this).

-Miles
--
Do not taunt Happy Fun Ball.

Re: Image cache

[Richard M. Stallman]

    But it is a bug that the image cache doesn't get updated when the image file
    content changes.  Is there an easy way to fix it?

Fixing this would be desirable.  One way to do it would
be to store the file's modtime in the image cache.
Then any reference to the cache could stat the file
to see if it has changed, and reload it if necessary.

That is pretty simple in theory.  Is it simple in practice?

Re: Image cache

[Miles Bader]

On 2/11/06, Richard M. Stallman <rms@gnu.org> wrote:
>     But it is a bug that the image cache doesn't get updated when the image file
>     content changes.  Is there an easy way to fix it?
>
> Fixing this would be desirable.  One way to do it would
> be to store the file's modtime in the image cache.
> Then any reference to the cache could stat the file
> to see if it has changed, and reload it if necessary.
>
> That is pretty simple in theory.  Is it simple in practice?

One certainly doesn't want to do a stat on _every_ reference, as stats
can be expensive (and the thumbs package will tend to use lots of
images); I think there would have to be some sort of counter/timer,
e.g., only stat if more than 30 seconds since the last stat.  However
as the stat info would give you time information anyway, that should
be relatively straightforward.

[Maybe the timeout should also  be an image parameter (for some uses,
e.g. icons in the mode-line, one may not care enough to pay the
expense of statting at all); one can also imagine an adaptive timeout,
which would stat more often if the file had been noticed to change
recently, which might work better for images being updated in the
background.]

-miles
--
Do not taunt Happy Fun Ball.

Re: Image cache

[Juri Linkov]

> One certainly doesn't want to do a stat on _every_ reference, as stats
> can be expensive (and the thumbs package will tend to use lots of
> images); I think there would have to be some sort of counter/timer,
> e.g., only stat if more than 30 seconds since the last stat.  However
> as the stat info would give you time information anyway, that should
> be relatively straightforward.

Maybe it would be enough to stat only on image creation.  Only this moment
is problematic now.  Emacs doesn't re-read the image from the file if the
file is cached by its name.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Image cache

[Chong Yidong]

Miles Bader <miles@gnu.org> writes:

> On 2/11/06, Richard M. Stallman <rms@gnu.org> wrote:
>>     But it is a bug that the image cache doesn't get updated when the image file
>>     content changes.  Is there an easy way to fix it?
>>
>> Fixing this would be desirable.  One way to do it would
>> be to store the file's modtime in the image cache.
>> Then any reference to the cache could stat the file
>> to see if it has changed, and reload it if necessary.
>>
>> That is pretty simple in theory.  Is it simple in practice?
>
> One certainly doesn't want to do a stat on _every_ reference, as stats
> can be expensive (and the thumbs package will tend to use lots of
> images)

I think a good way to do it is to provide a function that Lisp
programs can use to reload a specific cached image (rather than the
entire cache).

Re: Image cache

[Miles Bader]

Chong Yidong <cyd@stupidchicken.com> writes:
>> One certainly doesn't want to do a stat on _every_ reference, as stats
>> can be expensive (and the thumbs package will tend to use lots of
>> images)
>
> I think a good way to do it is to provide a function that Lisp
> programs can use to reload a specific cached image (rather than the
> entire cache).

That might a good function to have -- it's certainly better than simply
flushing the cache when you know you want to refresh a given image --
but it obviously doesn't solve quite the same problem.  A mode that
displays lots of images might like to be able to have them all update
auto-magically without having a bunch of elisp hair in place to keep
track of their status; the elisp solution would probably be clumsier and
less efficient than simply statting (periodically) upon redisplay.

-Miles
-- 
The secret to creativity is knowing how to hide your sources.
  --Albert Einstein

Re: Image cache

[Stefan Monnier]

> One certainly doesn't want to do a stat on _every_ reference, as stats
> can be expensive (and the thumbs package will tend to use lots of
> images); I think there would have to be some sort of counter/timer,
> e.g., only stat if more than 30 seconds since the last stat.  However
> as the stat info would give you time information anyway, that should
> be relatively straightforward.

The timeout is fairly arbitrary, so it's invariably turn out to be
unnecessarily short for half the cases and too long for the other half.

I much prefer an explicit way to force a check, which can be used in
create-image.  And in those rare cases where the image may change without
Emacs being directly involved, then a run-with-idle-timer can implement
the timeout.


        Stefan