bug#12463: 24.2; pos-visible-in-window-p gets slower over time

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Jörg Walter]

Emacs gets arbitrarily slow over time. I was able to narrow down the
problem to `pos-visible-in-window-p', which is, unfortunately, called as
part of lots of common commands. That function call is getting slower
each time it is called, if two conditions are met.

The first condition I was able to pin is the value of
`header-line-format'. The bug only occurs when it includes an image (as
is common when using tabbar.el).

It also depends on buffer contents, although I was not able to determine
a minimal condition. It happens with the fancy splash screen, but not
with the scratch buffer. I've tried inserting an image and using face
`variable-pitch', but those two aren't enough to trigger the bug.

This code sample demonstrates the bug. Run it in an "emacs -Q" instance
via `eval-buffer' and be amazed at the unbounded (linear) growth of
execution time for each iteration:

(defun bug ()
  "trigger bug related to pos-visible-in-window-p"
  (interactive)
  (benchmark 1000 '(pos-visible-in-window-p t)))

(fancy-startup-screen)
(setq header-line-format '((#("x" 0 1 (display (image :type pbm :data "P2 1 1 255\n"))))))
(goto-char (point-min))
(while t (bug))



In GNU Emacs 24.2.1 (x86_64-unknown-linux-gnu, GTK+ Version 2.24.10)
 of 2012-09-17 on queen
Windowing system distributor `The X.Org Foundation', version 11.0.11103000
Configured using:
 `configure '--with-gif=no''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Emacs-Lisp

Minor modes in effect:
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
C-x C-f ~ / . e m a c s . d / b u . <backspace> g . 
e l <return> M-x e v a l - b u f f e r <return> C-x 
k <return> M-x r e p o r t - e m <tab> <return>

Recent messages:
Invalid pixel value in image `(image :type pbm :data P2 1 1 255
)'
QuitInvalid pixel value in image `(image :type pbm :data P2 1 1 255
)'
Invalid pixel value in image `(image :type pbm :data P2 1 1 255
)'
Invalid pixel value in image `(image :type pbm :data P2 1 1 255
)'
Invalid pixel value in image `(image :type pbm :data P2 1 1 255
)'

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail regexp-opt rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils benchmark time-date tooltip
ediff-hook vc-hooks lisp-float-type mwheel x-win x-dnd tool-bar dnd
fontset image fringe lisp-mode register page menu-bar rfn-eshadow timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
frame cham georgian utf-8-lang misc-lang vietnamese tibetan thai
tai-viet lao korean japanese hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese case-table epa-hook jka-cmpr-hook help
simple abbrev minibuffer loaddefs button faces cus-face files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote make-network-process
dbusbind dynamic-setting system-font-setting font-render-setting
move-toolbar gtk x-toolkit x multi-tty emacs)

-- 
CU
  Jörg

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: jwalt@garni.ch (Jörg Walter)
> Date: Tue, 18 Sep 2012 01:51:31 +0200
> 
> Emacs gets arbitrarily slow over time. I was able to narrow down the
> problem to `pos-visible-in-window-p', which is, unfortunately, called as
> part of lots of common commands. That function call is getting slower
> each time it is called, if two conditions are met.
> 
> The first condition I was able to pin is the value of
> `header-line-format'. The bug only occurs when it includes an image (as
> is common when using tabbar.el).
> 
> It also depends on buffer contents, although I was not able to determine
> a minimal condition. It happens with the fancy splash screen, but not
> with the scratch buffer. I've tried inserting an image and using face
> `variable-pitch', but those two aren't enough to trigger the bug.
> 
> This code sample demonstrates the bug. Run it in an "emacs -Q" instance
> via `eval-buffer' and be amazed at the unbounded (linear) growth of
> execution time for each iteration:
> 
> (defun bug ()
>   "trigger bug related to pos-visible-in-window-p"
>   (interactive)
>   (benchmark 1000 '(pos-visible-in-window-p t)))
> 
> (fancy-startup-screen)
> (setq header-line-format '((#("x" 0 1 (display (image :type pbm :data "P2 1 1 255\n"))))))
> (goto-char (point-min))
> (while t (bug))

I cannot reproduce the slow-down on my machine (Windows XP).  What I
see is constant time, give or take 2%, with no growth trend
whatsoever.

I don't have access to GUI sessions on any configuration close to this:

> In GNU Emacs 24.2.1 (x86_64-unknown-linux-gnu, GTK+ Version 2.24.10)
>  of 2012-09-17 on queen
> Windowing system distributor `The X.Org Foundation', version 11.0.11103000
> Configured using:
>  `configure '--with-gif=no''

Can anyone else reproduce this?

Jörg, does this happen with earlier versions of Emacs, like 24.1 or
23.3?

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Jörg Walter]

Op den Dingsdag, de 18. September 2012 Klock 10:46:35 hett Eli Zaretskii 
schreven:
> I cannot reproduce the slow-down on my machine (Windows XP).  What I
> see is constant time, give or take 2%, with no growth trend
> whatsoever.
> 
> I don't have access to GUI sessions on any configuration close to this:
> > In GNU Emacs 24.2.1 (x86_64-unknown-linux-gnu, GTK+ Version 2.24.10)
> > 
> >  of 2012-09-17 on queen
> > 
> > Windowing system distributor `The X.Org Foundation', version 11.0.11103000
> > 
> > Configured using:
> >  `configure '--with-gif=no''
> 
> Can anyone else reproduce this?
> 
> Jörg, does this happen with earlier versions of Emacs, like 24.1 or
> 23.3?

I've done some additional tests: It does *not* happen with Ubuntu's emacs23 
(23.3.1). Just for cross-checking, I ran Win32 emacs 24.2 once via wine and 
once in VirtualBox+WinXP, in both cases no bug.

It *does* happen with Ubuntu's emacs24 (24.1.1), which is where I first noticed 
the problem. It also happens on all  X toolkits (gtk, gtk3, athena, lucid).

-- 
CU
	Jörg

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Jörg Walter <jwalt@garni.ch>
> Date: Tue, 18 Sep 2012 11:46:13 +0200
> 
> > Jörg, does this happen with earlier versions of Emacs, like 24.1 or
> > 23.3?
> 
> I've done some additional tests: It does *not* happen with Ubuntu's emacs23 
> (23.3.1). Just for cross-checking, I ran Win32 emacs 24.2 once via wine and 
> once in VirtualBox+WinXP, in both cases no bug.
> 
> It *does* happen with Ubuntu's emacs24 (24.1.1), which is where I first noticed 
> the problem. It also happens on all  X toolkits (gtk, gtk3, athena, lucid).

Thanks.

Would it be possible for you to time the related code on the C level,
and find which part(s) are responsible for the slow-down?

To do that, insert into the C code calls to a function that returns
some measure of time before and after the code fragment being timed,
and print the difference between two successive calls if that
difference exceeds some threshold.  For example:

  double
  timer_time (void)
  {
    struct timeval tv;

    gettimeofday (&tv, NULL);
    return tv.tv_usec * 0.000001 + tv.tv_sec;
  }
  [...]
    double t1, t2;
    extern double timer_time (void);
    [...]
    t1 = timer_time ();
    /* Some code being timed.  */
    t2 = timer_time ();
    if (t2 - t1 >= 0.001)  /* show time from 1 msec and up */
      {
	fprintf (stderr, "foo took %.4g sec\n", t2 - t1);
	fflush (stderr);
      }

The code in question is Fpos_visible_in_window_p itself, and its main
subroutine, pos_visible_p.  The latter is a large function, so if it
is the culprit, successively dividing it into smaller pieces by
bisection will allow you to show what part is taking the most time.

If you can do the above, the first thing to establish is whether
timing the whole of Fpos_visible_in_window_p on the C level shows
approximately the same times and exhibits the same growth trend as
what 'benchmark' shows on the Lisp level.  If it doesn't show the same
timings, then the code which implements pos-visible-in-window-p is not
the issue, and we should look elsewhere for the explanations.

Also, if you repeat the benchmark after terminating it, do the numbers
start from the last (longest) measurement or from the first
(shortest), or somewhere in between?  IOW, do you need to restart
Emacs to reset the measurements back to the first shortest value?

Thanks.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Matt Lundin]

Jörg Walter <jwalt@garni.ch> writes:

> I've done some additional tests: It does *not* happen with Ubuntu's
> emacs23 (23.3.1). Just for cross-checking, I ran Win32 emacs 24.2 once
> via wine and once in VirtualBox+WinXP, in both cases no bug.
>
> It *does* happen with Ubuntu's emacs24 (24.1.1), which is where I
> first noticed the problem. It also happens on all X toolkits (gtk,
> gtk3, athena, lucid).

Does it happen when using x instead of xft as the font backend? I have
found that xft rendering is sluggish in emacs 24 (on Arch Linux, in my
case). A single call to pos-visible-in-window-p can take as much as 0.3
secs according to ELP (e.g., when opening an outline heading, especially
if it contains multi-byte encodings). This slowdown does not occur when
xft is turned off (i.e, by placing "Emacs.FontBackend: x" in
.Xresources).

AFAICT, the bottleneck seems to be in the emacs xft rendering. I tested
this by opening two frames showing the same outline buffer. One of the
frames was a X frame using xft fonts; another frame was running in a
console (urxvt, which also uses xft rendering but shows no similar
slowdown). When opening an outline entry containing multibyte
characters, it appeared instantly in the console frame but only after a
substantial lag on the X frame.

In other words, the culprit may be something other than
pos-visible-in-window-p. For instance, when running emacs on x with xft
enabled, I find it can take as much as 3-4 seconds (according to elp) to
create a new frame with C-x 5 2. When running emacs on x with xft
disabled, new frames are created immediately.

Best,
Matt

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

> Can anyone else reproduce this?

I can reproduce this.  On bisecting, the problem first shows up with

  revno: 104186
  fixes bug: http://debbugs.gnu.org/8640
  committer: Juanma Barranquero <lekktu@gmail.com>
  branch nick: trunk
  timestamp: Tue 2011-05-10 12:31:33 +0200
  message:
  src/image.c (Finit_image_library): Return t for built-in image types.

A quick glance at the diff of this revision does not reveal immediately
why it causes the problem, though.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Jörg Walter]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

Op den Dingsdag, de 18. September 2012 Klock 23:17:53 hett Chong Yidong 
schreven:
> Eli Zaretskii <eliz@gnu.org> writes:
> > Can anyone else reproduce this?
> 
> I can reproduce this.  On bisecting, the problem first shows up with
> 
>   revno: 104186
>   fixes bug: http://debbugs.gnu.org/8640
>   committer: Juanma Barranquero <lekktu@gmail.com>
>   branch nick: trunk
>   timestamp: Tue 2011-05-10 12:31:33 +0200
>   message:
>   src/image.c (Finit_image_library): Return t for built-in image types.
> 
> A quick glance at the diff of this revision does not reveal immediately
> why it causes the problem, though.

I have no idea why that revision triggers the bug either, but here is a patch 
that fixes the problem. I have no idea if it matches your quality standards, 
but I think it is clean enough. I've reused and adapted some code already 
present for Win32 (which is the reason they don't suffer the same problem).

The linked list `image_types' grows without bounds because 
CHECK_LIB_AVAILABLE/define_image_type never checked if the given image type is 
already in `image_types'. My lisp code triggered tons of `Finit_image_library' 
calls. Since the list is searched linearly at some point, that explains the 
run-time impact.

I have no idea what was originally supposed to ensure `image_types' doesn't 
include duplicate entries, so the patch may be way off... hope someone actually 
knows what is suppsed to go on there.

-- 
CU
	Jörg

[-- Attachment #2: emacs-image-bug.patch --]
[-- Type: text/x-patch, Size: 1421 bytes --]

--- image.c	2012-09-18 18:10:21.952695267 +0200
+++ image.c.new	2012-09-18 18:09:18.173692841 +0200
@@ -575,12 +575,11 @@
 static void x_emboss (struct frame *, struct image *);
 static int x_build_heuristic_mask (struct frame *, struct image *,
 				   Lisp_Object);
-#ifdef HAVE_NTGUI
+
+static Lisp_Object loaded_image_types;
+
 #define CACHE_IMAGE_TYPE(type, status) \
-  do { Vlibrary_cache = Fcons (Fcons (type, status), Vlibrary_cache); } while (0)
-#else
-#define CACHE_IMAGE_TYPE(type, status)
-#endif
+  do { loaded_image_types = Fcons (Fcons (type, status), loaded_image_types); } while (0)
 
 #define ADD_IMAGE_TYPE(type) \
   do { Vimage_types = Fcons (type, Vimage_types); } while (0)
@@ -8743,12 +8742,10 @@
 of `dynamic-library-alist', which see).  */)
   (Lisp_Object type, Lisp_Object libraries)
 {
-#ifdef HAVE_NTGUI
   /* Don't try to reload the library.  */
-  Lisp_Object tested = Fassq (type, Vlibrary_cache);
+  Lisp_Object tested = Fassq (type, loaded_image_types);
   if (CONSP (tested))
     return XCDR (tested);
-#endif
 
   /* Types pbm and xbm are built-in and always available.  */
   if (EQ (type, Qpbm) || EQ (type, Qxbm))
@@ -8828,6 +8825,9 @@
 non-numeric, there is no explicit limit on the size of images.  */);
   Vmax_image_size = make_float (MAX_IMAGE_SIZE);
 
+  loaded_image_types = Qnil;
+  staticpro(&loaded_image_types);
+
   DEFSYM (Qpbm, "pbm");
   ADD_IMAGE_TYPE (Qpbm);
 

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Cc: jwalt@garni.ch (Jörg Walter),  12463@debbugs.gnu.org
> Date: Tue, 18 Sep 2012 23:17:53 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Can anyone else reproduce this?
> 
> I can reproduce this.  On bisecting, the problem first shows up with
> 
>   revno: 104186
>   fixes bug: http://debbugs.gnu.org/8640
>   committer: Juanma Barranquero <lekktu@gmail.com>
>   branch nick: trunk
>   timestamp: Tue 2011-05-10 12:31:33 +0200
>   message:
>   src/image.c (Finit_image_library): Return t for built-in image types.
> 
> A quick glance at the diff of this revision does not reveal immediately
> why it causes the problem, though.

Can you see what happened in revision 104105?  That was before the
previous image-handling code was modified into its present form, which
originally made pbm images unavailable (that was fixed in 104186).
The question is, was the recipe in this bug slow or fast then.
Knowing that will probably give a hint in the right direction.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Matt Lundin <mdl@imapmail.org>
> Date: Tue, 18 Sep 2012 10:05:34 -0500
> Cc: 12463@debbugs.gnu.org
> 
> AFAICT, the bottleneck seems to be in the emacs xft rendering. I tested
> this by opening two frames showing the same outline buffer. One of the
> frames was a X frame using xft fonts; another frame was running in a
> console (urxvt, which also uses xft rendering but shows no similar
> slowdown). When opening an outline entry containing multibyte
> characters, it appeared instantly in the console frame but only after a
> substantial lag on the X frame.
> 
> In other words, the culprit may be something other than
> pos-visible-in-window-p.

pos-visible-in-window-p invokes many functions involved in redisplay,
including the font shaper, it just arranges things so that nothing is
output to the glass.  So anything that slows down font shaping will
also slow down pos-visible-in-window-p.

However, in this case it looks like image drawing is the culprit, not
font shaping.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Jörg Walter]

Op den Dingsdag, de 18. September 2012 Klock 19:19:15 hett Eli Zaretskii 
schreven:
> Can you see what happened in revision 104105?  That was before the
> previous image-handling code was modified into its present form, which
> originally made pbm images unavailable (that was fixed in 104186).
> The question is, was the recipe in this bug slow or fast then.
> Knowing that will probably give a hint in the right direction.

Look at 104108. In a way, it is the reverse of my suggested patch. That at 
least explains something.

-- 
CU
	Jörg

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Jörg Walter <jwalt@garni.ch>
> Date: Tue, 18 Sep 2012 18:26:45 +0200
> 
> Op den Dingsdag, de 18. September 2012 Klock 19:19:15 hett Eli Zaretskii 
> schreven:
> > Can you see what happened in revision 104105?  That was before the
> > previous image-handling code was modified into its present form, which
> > originally made pbm images unavailable (that was fixed in 104186).
> > The question is, was the recipe in this bug slow or fast then.
> > Knowing that will probably give a hint in the right direction.
> 
> Look at 104108. In a way, it is the reverse of my suggested patch. That at 
> least explains something.

Right, that's why I asked Chong to test 104105.

I guess you already find the answer to that: the cache that was
removed for all the builds except w32.

Juanma, can you take care of this, please?

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Juanma Barranquero]

On Tue, Sep 18, 2012 at 7:19 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> Juanma, can you take care of this, please?

Assuming it can wait a few days, yes.

    Juanma

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 18 Sep 2012 19:31:22 +0200
> Cc: Jörg Walter <jwalt@garni.ch>, 12463@debbugs.gnu.org
> 
> On Tue, Sep 18, 2012 at 7:19 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > Juanma, can you take care of this, please?
> 
> Assuming it can wait a few days, yes.

Since Jörg published the solution, I don't see any special reasons to
hurry: anyone who needs it can apply the patches to their sandbox.

Thanks.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Juanma Barranquero]

On Tue, Sep 18, 2012 at 10:00 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> Since Jörg published the solution, I don't see any special reasons to
> hurry: anyone who needs it can apply the patches to their sandbox.

Jörg's fix solves the reported bug, but he's apparently unaware that
Vlibrary_cache is not just used for images, so renaming it to
loaded_image_types and making it private to image.c is a no-no.

The patch is trivial (Jörg's deletion of Windows-specific #ifdef's,
plus moving the declaration of Vlibrary_cache outside w32.c/h and to a
more general file). But I'm not sure where to put it; image.c is not
safe because you can have a non-HAVE_WINDOW_SYSTEM build supporting
libxml2 or GnuTLS.

Suggestions?

    Juanma

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 19 Sep 2012 04:31:44 +0200
> Cc: jwalt@garni.ch, 12463@debbugs.gnu.org
> 
> The patch is trivial (Jörg's deletion of Windows-specific #ifdef's,
> plus moving the declaration of Vlibrary_cache outside w32.c/h and to a
> more general file). But I'm not sure where to put it; image.c is not
> safe because you can have a non-HAVE_WINDOW_SYSTEM build supporting
> libxml2 or GnuTLS.
> 
> Suggestions?

emacs.c?

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Juanma Barranquero]

On Wed, Sep 19, 2012 at 4:57 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> emacs.c?

Yes, I've come to the same conclusion. Working on it.

    Juanma

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Juanma Barranquero]

On Tue, Sep 18, 2012 at 6:18 PM, Jörg Walter <jwalt@garni.ch> wrote:

> The linked list `image_types' grows without bounds because
> CHECK_LIB_AVAILABLE/define_image_type never checked if the given image type is
> already in `image_types'. My lisp code triggered tons of `Finit_image_library'
> calls. Since the list is searched linearly at some point, that explains the
> run-time impact.
>
> I have no idea what was originally supposed to ensure `image_types' doesn't
> include duplicate entries, so the patch may be way off... hope someone actually
> knows what is suppsed to go on there.

What kind of duplicate entries image_types had? Were they mostly (or
all) for pbm/xbm types?

    Juanma

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Chong Yidong]

Juanma Barranquero <lekktu@gmail.com> writes:

>> I have no idea what was originally supposed to ensure `image_types'
>> doesn't include duplicate entries, so the patch may be way
>> off... hope someone actually knows what is suppsed to go on there.
>
> What kind of duplicate entries image_types had? Were they mostly (or
> all) for pbm/xbm types?

No, the duplicates were for other image types other than pbm/xbm (in
this case svg).  The trouble, as Jörg pointed out, is that
define_image_types did not check for the prior existence of an image
type before registering it again.  An unfortunate oversight; I've
committed a fix to the trunk.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Chong Yidong]

Chong Yidong <cyd@gnu.org> writes:

>> What kind of duplicate entries image_types had? Were they mostly (or
>> all) for pbm/xbm types?
>
> No, the duplicates were for other image types other than pbm/xbm (in
> this case svg).  The trouble, as Jörg pointed out, is that
> define_image_types did not check for the prior existence of an image
> type before registering it again.  An unfortunate oversight; I've
> committed a fix to the trunk.

On further inspection of Finit_image_library and friends, the current
arrangement seems sub-optimal.  We have a Lisp-visible function
`init-image-library', which triggers loading of image types.  This
function is called by lookup_image_type, which afterwards scans the
image_types linked list again.

Please take a look at the following patch, which moves all the work done
by Finit_image_library into lookup_image_type.  This requires adding a
LIBRARIES argument to lookup_image_type, with the same meaning as
Finit_image_library and defaulting to Vdynamic_library_alist.  Then
Finit_image_library would be a rather simple wrapper around
lookup_image_type.  Other callers to lookup_image_type, such as
redisplay looking up an image, would be unaffected.

This patch also gets rid of the CHECK_LIB_AVAILABLE macro and replaces
it using a new slot in struct image_type, which stores the
initialization function for dynamic loading, if any.

This patch is orthogonal to the issue of moving Vlibrary_cache outside
w32.  One difference in behavior is that it makes the Windows code scan
Vlibrary_cache only after it has already failed to look for the image
type in image_types, and is about to run the initialization function.  I
think this is a bit more straightforward.


=== modified file 'src/dispextern.h'
*** src/dispextern.h	2012-09-13 02:21:28 +0000
--- src/dispextern.h	2012-09-21 05:15:36 +0000
***************
*** 2766,2771 ****
--- 2766,2774 ----
    /* Free resources of image IMG which is used on frame F.  */
    void (* free) (struct frame *f, struct image *img);
  
+   /* Initialization function, or NULL if none.  */
+   int (* init) (Lisp_Object);
+ 
    /* Next in list of all supported image types.  */
    struct image_type *next;
  };

=== modified file 'src/image.c'
*** src/image.c	2012-09-21 03:52:23 +0000
--- src/image.c	2012-09-21 05:35:31 +0000
***************
*** 561,568 ****
  
  /* Function prototypes.  */
  
! static Lisp_Object define_image_type (struct image_type *type, int loaded);
! static struct image_type *lookup_image_type (Lisp_Object symbol);
  static void image_error (const char *format, Lisp_Object, Lisp_Object);
  static void x_laplace (struct frame *, struct image *);
  static void x_emboss (struct frame *, struct image *);
--- 561,568 ----
  
  /* Function prototypes.  */
  
! static struct image_type *define_image_type (struct image_type *, Lisp_Object);
! static struct image_type *lookup_image_type (Lisp_Object, Lisp_Object);
  static void image_error (const char *format, Lisp_Object, Lisp_Object);
  static void x_laplace (struct frame *, struct image *);
  static void x_emboss (struct frame *, struct image *);
***************
*** 581,632 ****
  /* Define a new image type from TYPE.  This adds a copy of TYPE to
     image_types and caches the loading status of TYPE.  */
  
! static Lisp_Object
! define_image_type (struct image_type *type, int loaded)
  {
!   Lisp_Object success;
  
!   if (!loaded)
!     success = Qnil;
!   else
      {
!       struct image_type *p;
!       Lisp_Object target_type = *(type->type);
!       for (p = image_types; p; p = p->next)
!       	if (EQ (*(p->type), target_type))
!       	  return Qt;
  
        /* Make a copy of TYPE to avoid a bus error in a dumped Emacs.
           The initialized data segment is read-only.  */
        p = xmalloc (sizeof *p);
        *p = *type;
        p->next = image_types;
        image_types = p;
-       success = Qt;
      }
  
!   CACHE_IMAGE_TYPE (*type->type, success);
!   return success;
! }
! 
! 
! /* Look up image type SYMBOL, and return a pointer to its image_type
!    structure.  Value is null if SYMBOL is not a known image type.  */
! 
! static inline struct image_type *
! lookup_image_type (Lisp_Object symbol)
! {
!   struct image_type *type;
! 
!   /* We must initialize the image-type if it hasn't been already.  */
!   if (NILP (Finit_image_library (symbol, Vdynamic_library_alist)))
!     return 0;			/* unimplemented */
! 
!   for (type = image_types; type; type = type->next)
!     if (EQ (symbol, *type->type))
!       break;
! 
!   return type;
  }
  
  
--- 581,624 ----
  /* Define a new image type from TYPE.  This adds a copy of TYPE to
     image_types and caches the loading status of TYPE.  */
  
! static struct image_type *
! define_image_type (struct image_type *type, Lisp_Object libraries)
  {
!   struct image_type *p = NULL;
!   Lisp_Object target_type = *type->type;
!   int type_valid = 1;
! 
!   for (p = image_types; p; p = p->next)
!     if (EQ (*p->type, target_type))
!       return p;
  
!   if (type->init)
      {
! #ifdef HAVE_NTGUI
!       /* If we failed to load the library before, don't try again.  */
!       Lisp_Object tested = Fassq (target_type, Vlibrary_cache);
!       if (CONSP (tested) && NILP (XCDR (tested)))
! 	type_valid = 0;
!       else
! #endif
! 	{
! 	  /* If the load failed, avoid trying again.  */
! 	  type_valid = (*type->init)(libraries);
! 	  CACHE_IMAGE_TYPE (target_type, type_valid);
! 	}
!     }
  
+   if (type_valid)
+     {
        /* Make a copy of TYPE to avoid a bus error in a dumped Emacs.
           The initialized data segment is read-only.  */
        p = xmalloc (sizeof *p);
        *p = *type;
        p->next = image_types;
        image_types = p;
      }
  
!   return p;
  }
  
  
***************
*** 653,659 ****
  	    if (CONSP (tem) && SYMBOLP (XCAR (tem)))
  	      {
  		struct image_type *type;
! 		type = lookup_image_type (XCAR (tem));
  		if (type)
  		  valid_p = type->valid_p (object);
  	      }
--- 645,651 ----
  	    if (CONSP (tem) && SYMBOLP (XCAR (tem)))
  	      {
  		struct image_type *type;
! 		type = lookup_image_type (XCAR (tem), Qnil);
  		if (type)
  		  valid_p = type->valid_p (object);
  	      }
***************
*** 986,992 ****
  
    eassert (valid_image_p (spec));
    img->dependencies = NILP (file) ? Qnil : list1 (file);
!   img->type = lookup_image_type (image_spec_value (spec, QCtype, NULL));
    eassert (img->type != NULL);
    img->spec = spec;
    img->lisp_data = Qnil;
--- 978,984 ----
  
    eassert (valid_image_p (spec));
    img->dependencies = NILP (file) ? Qnil : list1 (file);
!   img->type = lookup_image_type (image_spec_value (spec, QCtype, NULL), Qnil);
    eassert (img->type != NULL);
    img->spec = spec;
    img->lisp_data = Qnil;
***************
*** 2262,2267 ****
--- 2254,2260 ----
    xbm_image_p,
    xbm_load,
    x_clear_image,
+   NULL,
    NULL
  };
  
***************
*** 3059,3064 ****
--- 3052,3062 ----
    xpm_image_p,
    xpm_load,
    x_clear_image,
+ #ifdef HAVE_NTGUI
+   init_xpm_functions,
+ #else
+   NULL,
+ #endif
    NULL
  };
  
***************
*** 4981,4986 ****
--- 4979,4985 ----
    pbm_image_p,
    pbm_load,
    x_clear_image,
+   NULL,
    NULL
  };
  
***************
*** 5395,5400 ****
--- 5394,5404 ----
    png_image_p,
    png_load,
    x_clear_image,
+ #ifdef HAVE_NTGUI
+   init_png_functions,
+ #else
+   NULL,
+ #endif
    NULL
  };
  
***************
*** 6047,6052 ****
--- 6051,6061 ----
    jpeg_image_p,
    jpeg_load,
    x_clear_image,
+ #ifdef HAVE_NTGUI
+   init_jpeg_functions,
+ #else
+   NULL,
+ #endif
    NULL
  };
  
***************
*** 6632,6637 ****
--- 6641,6651 ----
    tiff_image_p,
    tiff_load,
    x_clear_image,
+ #ifdef HAVE_NTGUI
+   init_tiff_functions,
+ #else
+   NULL,
+ #endif
    NULL
  };
  
***************
*** 7080,7085 ****
--- 7094,7104 ----
    gif_image_p,
    gif_load,
    gif_clear_image,
+ #ifdef HAVE_NTGUI
+   init_gif_functions,
+ #else
+   NULL,
+ #endif
    NULL
  };
  
***************
*** 7571,7576 ****
--- 7590,7600 ----
      imagemagick_image_p,
      imagemagick_load,
      imagemagick_clear_image,
+ #ifdef HAVE_NTGUI
+     init_imagemagick_functions,
+ #else
+     NULL,
+ #endif
      NULL
    };
  
***************
*** 8123,8128 ****
--- 8147,8157 ----
    svg_load,
    /* Handle to function to free sresources for SVG.  */
    x_clear_image,
+ #ifdef HAVE_NTGUI
+   init_svg_functions,
+ #else
+   NULL,
+ #endif
    /* An internal field to link to the next image type in a list of
       image types, will be filled in when registering the format.  */
    NULL
***************
*** 8512,8517 ****
--- 8541,8551 ----
    gs_image_p,
    gs_load,
    gs_clear_image,
+ #ifdef HAVE_NTGUI
+   init_gs_functions,
+ #else
+   NULL,
+ #endif
    NULL
  };
  
***************
*** 8774,8789 ****
  			    Initialization
   ***********************************************************************/
  
- #ifdef HAVE_NTGUI
- /* Image types that rely on external libraries are loaded dynamically
-    if the library is available.  */
- #define CHECK_LIB_AVAILABLE(image_type, init_lib_fn, libraries) \
-   define_image_type (image_type, init_lib_fn (libraries))
- #else
- #define CHECK_LIB_AVAILABLE(image_type, init_lib_fn, libraries) \
-   define_image_type (image_type, 1)
- #endif /* HAVE_NTGUI */
- 
  DEFUN ("init-image-library", Finit_image_library, Sinit_image_library, 2, 2, 0,
         doc: /* Initialize image library implementing image type TYPE.
  Return non-nil if TYPE is a supported image type.
--- 8808,8813 ----
***************
*** 8793,8853 ****
  of `dynamic-library-alist', which see).  */)
    (Lisp_Object type, Lisp_Object libraries)
  {
! #ifdef HAVE_NTGUI
!   /* Don't try to reload the library.  */
!   Lisp_Object tested = Fassq (type, Vlibrary_cache);
!   if (CONSP (tested))
!     return XCDR (tested);
! #endif
  
    /* Types pbm and xbm are built-in and always available.  */
!   if (EQ (type, Qpbm) || EQ (type, Qxbm))
!     return Qt;
  
  #if defined (HAVE_XPM) || defined (HAVE_NS)
    if (EQ (type, Qxpm))
!     return CHECK_LIB_AVAILABLE (&xpm_type, init_xpm_functions, libraries);
  #endif
  
  #if defined (HAVE_JPEG) || defined (HAVE_NS)
    if (EQ (type, Qjpeg))
!     return CHECK_LIB_AVAILABLE (&jpeg_type, init_jpeg_functions, libraries);
  #endif
  
  #if defined (HAVE_TIFF) || defined (HAVE_NS)
    if (EQ (type, Qtiff))
!     return CHECK_LIB_AVAILABLE (&tiff_type, init_tiff_functions, libraries);
  #endif
  
  #if defined (HAVE_GIF) || defined (HAVE_NS)
    if (EQ (type, Qgif))
!     return CHECK_LIB_AVAILABLE (&gif_type, init_gif_functions, libraries);
  #endif
  
  #if defined (HAVE_PNG) || defined (HAVE_NS)
    if (EQ (type, Qpng))
!     return CHECK_LIB_AVAILABLE (&png_type, init_png_functions, libraries);
  #endif
  
  #if defined (HAVE_RSVG)
    if (EQ (type, Qsvg))
!     return CHECK_LIB_AVAILABLE (&svg_type, init_svg_functions, libraries);
  #endif
  
  #if defined (HAVE_IMAGEMAGICK)
    if (EQ (type, Qimagemagick))
!     return CHECK_LIB_AVAILABLE (&imagemagick_type, init_imagemagick_functions,
!                                 libraries);
  #endif
  
  #ifdef HAVE_GHOSTSCRIPT
    if (EQ (type, Qpostscript))
!     return CHECK_LIB_AVAILABLE (&gs_type, init_gs_functions, libraries);
  #endif
  
!   /* If the type is not recognized, avoid testing it ever again.  */
!   CACHE_IMAGE_TYPE (type, Qnil);
!   return Qnil;
  }
  
  void
--- 8817,8883 ----
  of `dynamic-library-alist', which see).  */)
    (Lisp_Object type, Lisp_Object libraries)
  {
!   struct image_type *p = lookup_image_type (type, libraries);
!   return p ? *p->type : Qnil;
! }
! 
! /* Look up image type SYMBOL, and return a pointer to its image_type
!    structure.  Value is null if SYMBOL is not a known image type.  */
! 
! static struct image_type *
! lookup_image_type (Lisp_Object type, Lisp_Object libraries)
! {
!   if (NILP (libraries))
!     libraries = Vdynamic_library_alist;
  
    /* Types pbm and xbm are built-in and always available.  */
!   if (EQ (type, Qpbm))
!     return &pbm_type;
! 
!   if (EQ (type, Qxbm))
!     return &xbm_type;
  
  #if defined (HAVE_XPM) || defined (HAVE_NS)
    if (EQ (type, Qxpm))
!     return define_image_type (&xpm_type, libraries);
  #endif
  
  #if defined (HAVE_JPEG) || defined (HAVE_NS)
    if (EQ (type, Qjpeg))
!     return define_image_type (&jpeg_type, libraries);
  #endif
  
  #if defined (HAVE_TIFF) || defined (HAVE_NS)
    if (EQ (type, Qtiff))
!     return define_image_type (&tiff_type, libraries);
  #endif
  
  #if defined (HAVE_GIF) || defined (HAVE_NS)
    if (EQ (type, Qgif))
!     return define_image_type (&gif_type, libraries);
  #endif
  
  #if defined (HAVE_PNG) || defined (HAVE_NS)
    if (EQ (type, Qpng))
!     return define_image_type (&png_type, libraries);
  #endif
  
  #if defined (HAVE_RSVG)
    if (EQ (type, Qsvg))
!     return define_image_type (&svg_type, libraries);
  #endif
  
  #if defined (HAVE_IMAGEMAGICK)
    if (EQ (type, Qimagemagick))
!     return define_image_type (&imagemagick_type, libraries);
  #endif
  
  #ifdef HAVE_GHOSTSCRIPT
    if (EQ (type, Qpostscript))
!     return define_image_type (&gs_type, libraries);
  #endif
  
!   return NULL;
  }
  
  void

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Date: Fri, 21 Sep 2012 11:52:42 +0800
> Cc: Jörg Walter <jwalt@garni.ch>, 12463@debbugs.gnu.org
> 
> Juanma Barranquero <lekktu@gmail.com> writes:
> 
> >> I have no idea what was originally supposed to ensure `image_types'
> >> doesn't include duplicate entries, so the patch may be way
> >> off... hope someone actually knows what is suppsed to go on there.
> >
> > What kind of duplicate entries image_types had? Were they mostly (or
> > all) for pbm/xbm types?
> 
> No, the duplicates were for other image types other than pbm/xbm (in
> this case svg).

Then how come the bug was triggered by having a pbm image in the
header line?

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Date: Fri, 21 Sep 2012 13:42:53 +0800
> Cc: Jörg Walter <jwalt@garni.ch>, 12463@debbugs.gnu.org
> 
> Please take a look at the following patch, which moves all the work done
> by Finit_image_library into lookup_image_type.  This requires adding a
> LIBRARIES argument to lookup_image_type, with the same meaning as
> Finit_image_library and defaulting to Vdynamic_library_alist.  Then
> Finit_image_library would be a rather simple wrapper around
> lookup_image_type.  Other callers to lookup_image_type, such as
> redisplay looking up an image, would be unaffected.

Thanks.  Some comments below.

>   /* Define a new image type from TYPE.  This adds a copy of TYPE to
>      image_types and caches the loading status of TYPE.  */

The LIBRARIES argument should be documented in the commentary.

> ! static struct image_type *
> ! define_image_type (struct image_type *type, Lisp_Object libraries)
>   {

Since LIBRARIES could now be Qnil, but this function cannot tolerate
that, there should be an assertion to that effect, either here or in
every type->init function.

> ! #ifdef HAVE_NTGUI
> !       /* If we failed to load the library before, don't try again.  */
> !       Lisp_Object tested = Fassq (target_type, Vlibrary_cache);
> !       if (CONSP (tested) && NILP (XCDR (tested)))
> ! 	type_valid = 0;
> !       else
> ! #endif
> ! 	{
> ! 	  /* If the load failed, avoid trying again.  */
> ! 	  type_valid = (*type->init)(libraries);
> ! 	  CACHE_IMAGE_TYPE (target_type, type_valid);
> ! 	}
> !     }

What will happen if 'tested' is not a cons cell?

>   of `dynamic-library-alist', which see).  */)
>     (Lisp_Object type, Lisp_Object libraries)
>   {
> !   struct image_type *p = lookup_image_type (type, libraries);
> !   return p ? *p->type : Qnil;
> ! }

This changes the return value of init-image-library; is there a good
reason for not returning Qt here instead of the type symbol?

> ! /* Look up image type SYMBOL, and return a pointer to its image_type
> !    structure.  Value is null if SYMBOL is not a known image type.  */

Again, LIBRARIES is not documented.  Also, I believe we use NULL in
caps elsewhere.  And finally, the argument is called TYPE, not SYMBOL.

> ! static struct image_type *
> ! lookup_image_type (Lisp_Object type, Lisp_Object libraries)
> ! {
> !   if (NILP (libraries))
> !     libraries = Vdynamic_library_alist;

I can't say I like this "default".  Why not always call
lookup_image_type with Vdynamic_library_alist?  For that matter, why
not make lookup_image_type always use Vdynamic_library_alist without
passing it through the call parameters?

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Juanma Barranquero]

On Fri, Sep 21, 2012 at 5:52 AM, Chong Yidong <cyd@gnu.org> wrote:

> No, the duplicates were for other image types other than pbm/xbm (in
> this case svg).

Aha.

> The trouble, as Jörg pointed out, is that
> define_image_types did not check for the prior existence of an image
> type before registering it again.

Yes, that part I get. Eli and I were trying to understand why the bug
was triggered by displaying a pbm image, then.

> An unfortunate oversight; I've
> committed a fix to the trunk.

I had a patch ready...

    Juanma

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Juanma Barranquero]

On Fri, Sep 21, 2012 at 7:42 AM, Chong Yidong <cyd@gnu.org> wrote:

> ***************
> *** 3059,3064 ****
> --- 3052,3062 ----
>     xpm_image_p,
>     xpm_load,
>     x_clear_image,
> + #ifdef HAVE_NTGUI
> +   init_xpm_functions,
> + #else
> +   NULL,
> + #endif
>     NULL
>   };
>
> ***************

Additionally to Eli's comments, the functions init_*_functions are
defined after being used to initialize the struct, so it fails:

image.c:3056:3: error: 'init_xpm_functions' undeclared here (not in a function)
image.c:5398:3: error: 'init_png_functions' undeclared here (not in a function)
image.c:6055:3: error: 'init_jpeg_functions' undeclared here (not in a function)
image.c:6645:3: error: 'init_tiff_functions' undeclared here (not in a function)
image.c:7098:3: error: 'init_gif_functions' undeclared here (not in a function)

You'll have to move them around.

    Juanma

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Chong Yidong]

Juanma Barranquero <lekktu@gmail.com> writes:

>> An unfortunate oversight; I've
>> committed a fix to the trunk.
>
> I had a patch ready...

My apologies.  I assumed, from your previous messages, that you were
working on something orthogonal (moving the library cache stuff out of
w32).

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Juanma Barranquero]

On Fri, Sep 21, 2012 at 11:11 AM, Chong Yidong <cyd@gnu.org> wrote:

> I assumed, from your previous messages, that you were
> working on something orthogonal (moving the library cache stuff out of
> w32).

Both. But your fix is better than mine, so no worries.

Anyway, what I was doing to move Vlibrary_cache out of w32 is affected
by your patch, so I'd be happy if you could also fix that. The main
issue is making sure that dynamic loading can be used before dumping,
but the dumped Vlibrary_cache and image_types do not get dumped with
entries that would not be relevant afterwards (that's could be an
issue on Windows).

    Juanma

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

>> ! #ifdef HAVE_NTGUI
>> !       /* If we failed to load the library before, don't try again.  */
>> !       Lisp_Object tested = Fassq (target_type, Vlibrary_cache);
>> !       if (CONSP (tested) && NILP (XCDR (tested)))
>> ! 	type_valid = 0;
>> !       else
>> ! #endif
>> ! 	{
>> ! 	  /* If the load failed, avoid trying again.  */
>> ! 	  type_valid = (*type->init)(libraries);
>> ! 	  CACHE_IMAGE_TYPE (target_type, type_valid);
>> ! 	}
>> !     }
>
> What will happen if 'tested' is not a cons cell?

If `tested' is not a cons cell, it must be nil, and the code proceeds
with the library initialization.

>>   of `dynamic-library-alist', which see).  */)
>>     (Lisp_Object type, Lisp_Object libraries)
>>   {
>> !   struct image_type *p = lookup_image_type (type, libraries);
>> !   return p ? *p->type : Qnil;
>> ! }
>
> This changes the return value of init-image-library; is there a good
> reason for not returning Qt here instead of the type symbol?

No good reason; we could return Qt here.

>> ! /* Look up image type SYMBOL, and return a pointer to its image_type
>> !    structure.  Value is null if SYMBOL is not a known image type.  */
>
> Again, LIBRARIES is not documented.  Also, I believe we use NULL in
> caps elsewhere.  And finally, the argument is called TYPE, not SYMBOL.

Thanks.

>> ! static struct image_type *
>> ! lookup_image_type (Lisp_Object type, Lisp_Object libraries)
>> ! {
>> !   if (NILP (libraries))
>> !     libraries = Vdynamic_library_alist;
>
> I can't say I like this "default".  Why not always call
> lookup_image_type with Vdynamic_library_alist?  For that matter, why
> not make lookup_image_type always use Vdynamic_library_alist without
> passing it through the call parameters?

This is simply following the existing practice of Finit_image_library.
I don't know why that function accepts a LIBRARIES argument, rather than
just making callers bind Vdynamic_library_alist.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Chong Yidong]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

Juanma Barranquero <lekktu@gmail.com> writes:

> Additionally to Eli's comments, the functions init_*_functions are
> defined after being used to initialize the struct, so it fails:
>
> You'll have to move them around.

Thanks.  Fixed in the attached patch, which also addresses Eli's other
comments.  Please take a look again.

> Anyway, what I was doing to move Vlibrary_cache out of w32 is affected
> by your patch, so I'd be happy if you could also fix that. The main
> issue is making sure that dynamic loading can be used before dumping,
> but the dumped Vlibrary_cache and image_types do not get dumped with
> entries that would not be relevant afterwards (that's could be an
> issue on Windows).

As far as I can tell, my patch should not change anything in this
regard, except that when you move Vlibrary_cache out of w32, you can
also get rid of the #ifdef NTGUI in define_image_type.


Eli Zaretskii <eliz@gnu.org> writes:

> Since LIBRARIES could now be Qnil, but this function cannot tolerate
> that, there should be an assertion to that effect, either here or in
> every type->init function.

Passing a nil LIBRARIES argument causes nil to be passed to
w32_delayed_load, which AFAICT is safe; w32_delayed_load deals with nil
just fine, as a no-op.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: image.patch --]
[-- Type: text/x-diff, Size: 15163 bytes --]

=== modified file 'src/dispextern.h'
*** src/dispextern.h	2012-09-13 02:21:28 +0000
--- src/dispextern.h	2012-09-21 09:29:26 +0000
***************
*** 2766,2771 ****
--- 2766,2775 ----
    /* Free resources of image IMG which is used on frame F.  */
    void (* free) (struct frame *f, struct image *img);
  
+   /* Initialization function (used for dynamic loading of image
+      libraries on Windows), or NULL if none.  */
+   int (* init) (Lisp_Object);
+ 
    /* Next in list of all supported image types.  */
    struct image_type *next;
  };

=== modified file 'src/image.c'
*** src/image.c	2012-09-21 03:52:23 +0000
--- src/image.c	2012-09-21 09:47:34 +0000
***************
*** 561,568 ****
  
  /* Function prototypes.  */
  
! static Lisp_Object define_image_type (struct image_type *type, int loaded);
! static struct image_type *lookup_image_type (Lisp_Object symbol);
  static void image_error (const char *format, Lisp_Object, Lisp_Object);
  static void x_laplace (struct frame *, struct image *);
  static void x_emboss (struct frame *, struct image *);
--- 561,568 ----
  
  /* Function prototypes.  */
  
! static struct image_type *define_image_type (struct image_type *, Lisp_Object);
! static struct image_type *lookup_image_type (Lisp_Object, Lisp_Object);
  static void image_error (const char *format, Lisp_Object, Lisp_Object);
  static void x_laplace (struct frame *, struct image *);
  static void x_emboss (struct frame *, struct image *);
***************
*** 579,632 ****
    do { Vimage_types = Fcons (type, Vimage_types); } while (0)
  
  /* Define a new image type from TYPE.  This adds a copy of TYPE to
!    image_types and caches the loading status of TYPE.  */
  
! static Lisp_Object
! define_image_type (struct image_type *type, int loaded)
  {
!   Lisp_Object success;
  
!   if (!loaded)
!     success = Qnil;
!   else
      {
!       struct image_type *p;
!       Lisp_Object target_type = *(type->type);
!       for (p = image_types; p; p = p->next)
!       	if (EQ (*(p->type), target_type))
!       	  return Qt;
  
        /* Make a copy of TYPE to avoid a bus error in a dumped Emacs.
           The initialized data segment is read-only.  */
        p = xmalloc (sizeof *p);
        *p = *type;
        p->next = image_types;
        image_types = p;
-       success = Qt;
      }
  
!   CACHE_IMAGE_TYPE (*type->type, success);
!   return success;
! }
! 
! 
! /* Look up image type SYMBOL, and return a pointer to its image_type
!    structure.  Value is null if SYMBOL is not a known image type.  */
! 
! static inline struct image_type *
! lookup_image_type (Lisp_Object symbol)
! {
!   struct image_type *type;
! 
!   /* We must initialize the image-type if it hasn't been already.  */
!   if (NILP (Finit_image_library (symbol, Vdynamic_library_alist)))
!     return 0;			/* unimplemented */
! 
!   for (type = image_types; type; type = type->next)
!     if (EQ (symbol, *type->type))
!       break;
! 
!   return type;
  }
  
  
--- 579,629 ----
    do { Vimage_types = Fcons (type, Vimage_types); } while (0)
  
  /* Define a new image type from TYPE.  This adds a copy of TYPE to
!    image_types and caches the loading status of TYPE.
  
!    LIBRARIES is an alist associating dynamic libraries to external
!    files implementing them, which is passed to the image library
!    initialization function if necessary.  A nil value defaults to
!    Vdynamic_library_alist.  */
! 
! static struct image_type *
! define_image_type (struct image_type *type, Lisp_Object libraries)
  {
!   struct image_type *p = NULL;
!   Lisp_Object target_type = *type->type;
!   int type_valid = 1;
  
!   for (p = image_types; p; p = p->next)
!     if (EQ (*p->type, target_type))
!       return p;
! 
!   if (type->init)
      {
! #ifdef HAVE_NTGUI
!       /* If we failed to load the library before, don't try again.  */
!       Lisp_Object tested = Fassq (target_type, Vlibrary_cache);
!       if (CONSP (tested) && NILP (XCDR (tested)))
! 	type_valid = 0;
!       else
! #endif
! 	{
! 	  /* If the load failed, avoid trying again.  */
! 	  type_valid = (*type->init)(libraries);
! 	  CACHE_IMAGE_TYPE (target_type, type_valid);
! 	}
!     }
  
+   if (type_valid)
+     {
        /* Make a copy of TYPE to avoid a bus error in a dumped Emacs.
           The initialized data segment is read-only.  */
        p = xmalloc (sizeof *p);
        *p = *type;
        p->next = image_types;
        image_types = p;
      }
  
!   return p;
  }
  
  
***************
*** 653,659 ****
  	    if (CONSP (tem) && SYMBOLP (XCAR (tem)))
  	      {
  		struct image_type *type;
! 		type = lookup_image_type (XCAR (tem));
  		if (type)
  		  valid_p = type->valid_p (object);
  	      }
--- 650,656 ----
  	    if (CONSP (tem) && SYMBOLP (XCAR (tem)))
  	      {
  		struct image_type *type;
! 		type = lookup_image_type (XCAR (tem), Qnil);
  		if (type)
  		  valid_p = type->valid_p (object);
  	      }
***************
*** 986,992 ****
  
    eassert (valid_image_p (spec));
    img->dependencies = NILP (file) ? Qnil : list1 (file);
!   img->type = lookup_image_type (image_spec_value (spec, QCtype, NULL));
    eassert (img->type != NULL);
    img->spec = spec;
    img->lisp_data = Qnil;
--- 983,989 ----
  
    eassert (valid_image_p (spec));
    img->dependencies = NILP (file) ? Qnil : list1 (file);
!   img->type = lookup_image_type (image_spec_value (spec, QCtype, NULL), Qnil);
    eassert (img->type != NULL);
    img->spec = spec;
    img->lisp_data = Qnil;
***************
*** 2262,2267 ****
--- 2259,2265 ----
    xbm_image_p,
    xbm_load,
    x_clear_image,
+   NULL,
    NULL
  };
  
***************
*** 3051,3056 ****
--- 3049,3060 ----
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_xpm_functions (Lisp_Object);
+ #else
+ #define init_xpm_functions NULL
+ #endif
+ 
  /* Structure describing the image type XPM.  */
  
  static struct image_type xpm_type =
***************
*** 3059,3064 ****
--- 3063,3069 ----
    xpm_image_p,
    xpm_load,
    x_clear_image,
+   init_xpm_functions,
    NULL
  };
  
***************
*** 4981,4986 ****
--- 4986,4992 ----
    pbm_image_p,
    pbm_load,
    x_clear_image,
+   NULL,
    NULL
  };
  
***************
*** 5387,5392 ****
--- 5393,5404 ----
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_png_functions (Lisp_Object);
+ #else
+ #define init_png_functions NULL
+ #endif
+ 
  /* Structure describing the image type `png'.  */
  
  static struct image_type png_type =
***************
*** 5395,5400 ****
--- 5407,5413 ----
    png_image_p,
    png_load,
    x_clear_image,
+   init_png_functions,
    NULL
  };
  
***************
*** 6039,6044 ****
--- 6052,6063 ----
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_jpeg_functions (Lisp_Object);
+ #else
+ #define init_jpeg_functions NULL
+ #endif
+ 
  /* Structure describing the image type `jpeg'.  */
  
  static struct image_type jpeg_type =
***************
*** 6047,6052 ****
--- 6066,6072 ----
    jpeg_image_p,
    jpeg_load,
    x_clear_image,
+   init_jpeg_functions,
    NULL
  };
  
***************
*** 6624,6629 ****
--- 6644,6655 ----
    {":index",		IMAGE_NON_NEGATIVE_INTEGER_VALUE,	0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_tiff_functions (Lisp_Object);
+ #else
+ #define init_tiff_functions NULL
+ #endif
+ 
  /* Structure describing the image type `tiff'.  */
  
  static struct image_type tiff_type =
***************
*** 6632,6637 ****
--- 6658,6664 ----
    tiff_image_p,
    tiff_load,
    x_clear_image,
+   init_tiff_functions,
    NULL
  };
  
***************
*** 7072,7077 ****
--- 7099,7110 ----
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_gif_functions (Lisp_Object);
+ #else
+ #define init_gif_functions NULL
+ #endif
+ 
  /* Structure describing the image type `gif'.  */
  
  static struct image_type gif_type =
***************
*** 7080,7085 ****
--- 7113,7119 ----
    gif_image_p,
    gif_load,
    gif_clear_image,
+   init_gif_functions,
    NULL
  };
  
***************
*** 7562,7567 ****
--- 7596,7607 ----
      {":crop",		IMAGE_DONT_CHECK_VALUE_TYPE,		0}
    };
  
+ #ifdef HAVE_NTGUI
+ static int init_imagemagick_functions (Lisp_Object);
+ #else
+ #define init_imagemagick_functions NULL
+ #endif
+ 
  /* Structure describing the image type for any image handled via
     ImageMagick.  */
  
***************
*** 7571,7576 ****
--- 7611,7617 ----
      imagemagick_image_p,
      imagemagick_load,
      imagemagick_clear_image,
+     init_imagemagick_functions,
      NULL
    };
  
***************
*** 8109,8130 ****
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
  /* Structure describing the image type `svg'.  Its the same type of
     structure defined for all image formats, handled by emacs image
     functions.  See struct image_type in dispextern.h.  */
  
  static struct image_type svg_type =
  {
-   /* An identifier showing that this is an image structure for the SVG format.  */
    &Qsvg,
-   /* Handle to a function that can be used to identify a SVG file.  */
    svg_image_p,
-   /* Handle to function used to load a SVG file.  */
    svg_load,
-   /* Handle to function to free sresources for SVG.  */
    x_clear_image,
!   /* An internal field to link to the next image type in a list of
!      image types, will be filled in when registering the format.  */
    NULL
  };
  
--- 8150,8172 ----
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_svg_functions (Lisp_Object);
+ #else
+ #define init_svg_functions NULL
+ #endif
+ 
  /* Structure describing the image type `svg'.  Its the same type of
     structure defined for all image formats, handled by emacs image
     functions.  See struct image_type in dispextern.h.  */
  
  static struct image_type svg_type =
  {
    &Qsvg,
    svg_image_p,
    svg_load,
    x_clear_image,
!   init_svg_functions,
    NULL
  };
  
***************
*** 8504,8509 ****
--- 8546,8557 ----
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_gs_functions (Lisp_Object);
+ #else
+ #define init_gs_functions NULL
+ #endif
+ 
  /* Structure describing the image type `ghostscript'.  */
  
  static struct image_type gs_type =
***************
*** 8512,8517 ****
--- 8560,8566 ----
    gs_image_p,
    gs_load,
    gs_clear_image,
+   init_gs_functions,
    NULL
  };
  
***************
*** 8774,8789 ****
  			    Initialization
   ***********************************************************************/
  
- #ifdef HAVE_NTGUI
- /* Image types that rely on external libraries are loaded dynamically
-    if the library is available.  */
- #define CHECK_LIB_AVAILABLE(image_type, init_lib_fn, libraries) \
-   define_image_type (image_type, init_lib_fn (libraries))
- #else
- #define CHECK_LIB_AVAILABLE(image_type, init_lib_fn, libraries) \
-   define_image_type (image_type, 1)
- #endif /* HAVE_NTGUI */
- 
  DEFUN ("init-image-library", Finit_image_library, Sinit_image_library, 2, 2, 0,
         doc: /* Initialize image library implementing image type TYPE.
  Return non-nil if TYPE is a supported image type.
--- 8823,8828 ----
***************
*** 8793,8853 ****
  of `dynamic-library-alist', which see).  */)
    (Lisp_Object type, Lisp_Object libraries)
  {
! #ifdef HAVE_NTGUI
!   /* Don't try to reload the library.  */
!   Lisp_Object tested = Fassq (type, Vlibrary_cache);
!   if (CONSP (tested))
!     return XCDR (tested);
! #endif
  
    /* Types pbm and xbm are built-in and always available.  */
!   if (EQ (type, Qpbm) || EQ (type, Qxbm))
!     return Qt;
  
  #if defined (HAVE_XPM) || defined (HAVE_NS)
    if (EQ (type, Qxpm))
!     return CHECK_LIB_AVAILABLE (&xpm_type, init_xpm_functions, libraries);
  #endif
  
  #if defined (HAVE_JPEG) || defined (HAVE_NS)
    if (EQ (type, Qjpeg))
!     return CHECK_LIB_AVAILABLE (&jpeg_type, init_jpeg_functions, libraries);
  #endif
  
  #if defined (HAVE_TIFF) || defined (HAVE_NS)
    if (EQ (type, Qtiff))
!     return CHECK_LIB_AVAILABLE (&tiff_type, init_tiff_functions, libraries);
  #endif
  
  #if defined (HAVE_GIF) || defined (HAVE_NS)
    if (EQ (type, Qgif))
!     return CHECK_LIB_AVAILABLE (&gif_type, init_gif_functions, libraries);
  #endif
  
  #if defined (HAVE_PNG) || defined (HAVE_NS)
    if (EQ (type, Qpng))
!     return CHECK_LIB_AVAILABLE (&png_type, init_png_functions, libraries);
  #endif
  
  #if defined (HAVE_RSVG)
    if (EQ (type, Qsvg))
!     return CHECK_LIB_AVAILABLE (&svg_type, init_svg_functions, libraries);
  #endif
  
  #if defined (HAVE_IMAGEMAGICK)
    if (EQ (type, Qimagemagick))
!     return CHECK_LIB_AVAILABLE (&imagemagick_type, init_imagemagick_functions,
!                                 libraries);
  #endif
  
  #ifdef HAVE_GHOSTSCRIPT
    if (EQ (type, Qpostscript))
!     return CHECK_LIB_AVAILABLE (&gs_type, init_gs_functions, libraries);
  #endif
  
!   /* If the type is not recognized, avoid testing it ever again.  */
!   CACHE_IMAGE_TYPE (type, Qnil);
!   return Qnil;
  }
  
  void
--- 8832,8902 ----
  of `dynamic-library-alist', which see).  */)
    (Lisp_Object type, Lisp_Object libraries)
  {
!   return lookup_image_type (type, libraries) ? Qt : Qnil;
! }
! 
! /* Look up image type TYPE, and return a pointer to its image_type
!    structure.  Return 0 if TYPE is not a known image type.
! 
!    LIBRARIES is an alist associating dynamic libraries to external
!    files implementing them, which is passed to the image library
!    initialization function if necessary.  A nil value defaults to
!    Vdynamic_library_alist.  */
! 
! static struct image_type *
! lookup_image_type (Lisp_Object type, Lisp_Object libraries)
! {
!   if (NILP (libraries))
!     libraries = Vdynamic_library_alist;
  
    /* Types pbm and xbm are built-in and always available.  */
!   if (EQ (type, Qpbm))
!     return &pbm_type;
! 
!   if (EQ (type, Qxbm))
!     return &xbm_type;
  
  #if defined (HAVE_XPM) || defined (HAVE_NS)
    if (EQ (type, Qxpm))
!     return define_image_type (&xpm_type, libraries);
  #endif
  
  #if defined (HAVE_JPEG) || defined (HAVE_NS)
    if (EQ (type, Qjpeg))
!     return define_image_type (&jpeg_type, libraries);
  #endif
  
  #if defined (HAVE_TIFF) || defined (HAVE_NS)
    if (EQ (type, Qtiff))
!     return define_image_type (&tiff_type, libraries);
  #endif
  
  #if defined (HAVE_GIF) || defined (HAVE_NS)
    if (EQ (type, Qgif))
!     return define_image_type (&gif_type, libraries);
  #endif
  
  #if defined (HAVE_PNG) || defined (HAVE_NS)
    if (EQ (type, Qpng))
!     return define_image_type (&png_type, libraries);
  #endif
  
  #if defined (HAVE_RSVG)
    if (EQ (type, Qsvg))
!     return define_image_type (&svg_type, libraries);
  #endif
  
  #if defined (HAVE_IMAGEMAGICK)
    if (EQ (type, Qimagemagick))
!     return define_image_type (&imagemagick_type, libraries);
  #endif
  
  #ifdef HAVE_GHOSTSCRIPT
    if (EQ (type, Qpostscript))
!     return define_image_type (&gs_type, libraries);
  #endif
  
!   return NULL;
  }
  
  void

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Juanma Barranquero]

On Fri, Sep 21, 2012 at 11:24 AM, Chong Yidong <cyd@gnu.org> wrote:

> I don't know why that function accepts a LIBRARIES argument, rather than
> just making callers bind Vdynamic_library_alist.

That's what it did in the original design, but someone (Stefan, I
think) asked that it was passed as an argument. I have always disliked
that, as it seems highly unlikely that any other variable will ever be
used. I'd be happy to see that argument removed.

    Juanma

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Fri, 21 Sep 2012 12:47:56 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, jwalt@garni.ch, 12463@debbugs.gnu.org
> 
> On Fri, Sep 21, 2012 at 11:24 AM, Chong Yidong <cyd@gnu.org> wrote:
> 
> > I don't know why that function accepts a LIBRARIES argument, rather than
> > just making callers bind Vdynamic_library_alist.
> 
> That's what it did in the original design, but someone (Stefan, I
> think) asked that it was passed as an argument. I have always disliked
> that, as it seems highly unlikely that any other variable will ever be
> used. I'd be happy to see that argument removed.

Well, then maybe Stefan could explain why he wanted it.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Stefan Monnier]

>> > I don't know why that function accepts a LIBRARIES argument, rather than
>> > just making callers bind Vdynamic_library_alist.
>> That's what it did in the original design, but someone (Stefan, I
>> think) asked that it was passed as an argument. I have always disliked
>> that, as it seems highly unlikely that any other variable will ever be
>> used. I'd be happy to see that argument removed.
> Well, then maybe Stefan could explain why he wanted it.

Sorry, can't remember.  Probably because I generally prefer passing
arguments explicitly over passing them via dynamic binding.


        Stefan

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Juanma Barranquero <lekktu@gmail.com>, cyd@gnu.org, jwalt@garni.ch,
>         12463@debbugs.gnu.org
> Date: Fri, 21 Sep 2012 12:38:25 -0400
> 
> >> > I don't know why that function accepts a LIBRARIES argument, rather than
> >> > just making callers bind Vdynamic_library_alist.
> >> That's what it did in the original design, but someone (Stefan, I
> >> think) asked that it was passed as an argument. I have always disliked
> >> that, as it seems highly unlikely that any other variable will ever be
> >> used. I'd be happy to see that argument removed.
> > Well, then maybe Stefan could explain why he wanted it.
> 
> Sorry, can't remember.  Probably because I generally prefer passing
> arguments explicitly over passing them via dynamic binding.

We have gobs of functions in Emacs that access global variables like
Vdynamic_library_alist.  I don't think one more function will change
anything.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Date: Fri, 21 Sep 2012 18:01:19 +0800
> Cc: Jörg Walter <jwalt@garni.ch>, 12463@debbugs.gnu.org
> 
> Thanks.  Fixed in the attached patch, which also addresses Eli's other
> comments.  Please take a look again.

Thanks.

>   /* Define a new image type from TYPE.  This adds a copy of TYPE to
> !    image_types and caches the loading status of TYPE.
>   
> !    LIBRARIES is an alist associating dynamic libraries to external
> !    files implementing them, which is passed to the image library
> !    initialization function if necessary.  A nil value defaults to
> !    Vdynamic_library_alist.  */
> ! 
> ! static struct image_type *
> ! define_image_type (struct image_type *type, Lisp_Object libraries)
>   {
> !   struct image_type *p = NULL;
> !   Lisp_Object target_type = *type->type;
> !   int type_valid = 1;
>   
> !   for (p = image_types; p; p = p->next)
> !     if (EQ (*p->type, target_type))
> !       return p;
> ! 
> !   if (type->init)
>       {
> ! #ifdef HAVE_NTGUI
> !       /* If we failed to load the library before, don't try again.  */
> !       Lisp_Object tested = Fassq (target_type, Vlibrary_cache);
> !       if (CONSP (tested) && NILP (XCDR (tested)))
> ! 	type_valid = 0;
> !       else
> ! #endif
> ! 	{
> ! 	  /* If the load failed, avoid trying again.  */
> ! 	  type_valid = (*type->init)(libraries);
> ! 	  CACHE_IMAGE_TYPE (target_type, type_valid);
> ! 	}
> !     }
>   
> +   if (type_valid)
> +     {
>         /* Make a copy of TYPE to avoid a bus error in a dumped Emacs.
>            The initialized data segment is read-only.  */
>         p = xmalloc (sizeof *p);
>         *p = *type;
>         p->next = image_types;
>         image_types = p;
>       }
>   
> !   return p;
>   }

Am I missing something, or will this really call the initialization
function again and again and again?  AFAIU, if an image's library is
already in Vlibrary_cache, and the cdr of its association is non-nil,
that means the initialization function was already called for the
corresponding image type, and it returned non-zero.  So there's no
need to call it again.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Juanma Barranquero]

On Fri, Sep 21, 2012 at 7:03 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> Am I missing something, or will this really call the initialization
> function again and again and again?  AFAIU, if an image's library is
> already in Vlibrary_cache, and the cdr of its association is non-nil,
> that means the initialization function was already called for the
> corresponding image type, and it returned non-zero.  So there's no
> need to call it again.

In fact, if an image library is already in Vlibrary_cache, and the cdr
is nil, that also means that the initialization function was called
for that image type, if failed, and should never be called again.

    Juanma

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Fri, 21 Sep 2012 19:07:28 +0200
> Cc: Chong Yidong <cyd@gnu.org>, jwalt@garni.ch, 12463@debbugs.gnu.org
> 
> On Fri, Sep 21, 2012 at 7:03 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > Am I missing something, or will this really call the initialization
> > function again and again and again?  AFAIU, if an image's library is
> > already in Vlibrary_cache, and the cdr of its association is non-nil,
> > that means the initialization function was already called for the
> > corresponding image type, and it returned non-zero.  So there's no
> > need to call it again.
> 
> In fact, if an image library is already in Vlibrary_cache, and the cdr
> is nil, that also means that the initialization function was called
> for that image type, if failed, and should never be called again.

That part is OK: if the cdr of the association is nil, the
initialization function is not called.

And I see now that image_types is searched before looking in
Vlibrary_cache, so the answer to my question is "yes, I did miss
something".

IOW, Vlibrary_cache is only searched to detect a previous unsuccessful
attempt to load the library, in which case we don't try again.
Otherwise, the image type will either be found in image_types, or
loaded by calling the initialization function.  So I guess this is
okay, sorry for the noise.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Stefan Monnier]

>> Sorry, can't remember.  Probably because I generally prefer passing
>> arguments explicitly over passing them via dynamic binding.
> We have gobs of functions in Emacs that access global variables like
> Vdynamic_library_alist.  I don't think one more function will change
> anything.

The question is not "global or not" but "let-bound or not".
If it's generally accessed as a global var, that's perfectly fine.
If it's generally let-bound, I prefer an explicit argument.


        Stefan

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Chong Yidong]

[-- Attachment #1: Type: text/plain, Size: 340 bytes --]

Juanma helped me spot a few more errors in the patch off-list.  Here's
another iteration.  I added a BLOCK_INPUT in define_image_type; this
seems necessary because lookup_image_type can be called with input
unblocked.

If further testing does not turn up any issues, I'll commit it so that
the Vlibrary_cache refactoring work can proceed.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: image.patch --]
[-- Type: text/x-diff, Size: 15581 bytes --]

=== modified file 'src/dispextern.h'
*** src/dispextern.h	2012-09-13 02:21:28 +0000
--- src/dispextern.h	2012-09-21 09:29:26 +0000
***************
*** 2766,2771 ****
--- 2766,2775 ----
    /* Free resources of image IMG which is used on frame F.  */
    void (* free) (struct frame *f, struct image *img);
  
+   /* Initialization function (used for dynamic loading of image
+      libraries on Windows), or NULL if none.  */
+   int (* init) (Lisp_Object);
+ 
    /* Next in list of all supported image types.  */
    struct image_type *next;
  };

=== modified file 'src/image.c'
*** src/image.c	2012-09-21 03:52:23 +0000
--- src/image.c	2012-09-22 01:19:13 +0000
***************
*** 561,568 ****
  
  /* Function prototypes.  */
  
! static Lisp_Object define_image_type (struct image_type *type, int loaded);
! static struct image_type *lookup_image_type (Lisp_Object symbol);
  static void image_error (const char *format, Lisp_Object, Lisp_Object);
  static void x_laplace (struct frame *, struct image *);
  static void x_emboss (struct frame *, struct image *);
--- 561,568 ----
  
  /* Function prototypes.  */
  
! static struct image_type *define_image_type (struct image_type *, Lisp_Object);
! static struct image_type *lookup_image_type (Lisp_Object, Lisp_Object);
  static void image_error (const char *format, Lisp_Object, Lisp_Object);
  static void x_laplace (struct frame *, struct image *);
  static void x_emboss (struct frame *, struct image *);
***************
*** 579,632 ****
    do { Vimage_types = Fcons (type, Vimage_types); } while (0)
  
  /* Define a new image type from TYPE.  This adds a copy of TYPE to
!    image_types and caches the loading status of TYPE.  */
  
! static Lisp_Object
! define_image_type (struct image_type *type, int loaded)
! {
!   Lisp_Object success;
  
!   if (!loaded)
!     success = Qnil;
!   else
      {
!       struct image_type *p;
!       Lisp_Object target_type = *(type->type);
!       for (p = image_types; p; p = p->next)
!       	if (EQ (*(p->type), target_type))
!       	  return Qt;
  
        /* Make a copy of TYPE to avoid a bus error in a dumped Emacs.
           The initialized data segment is read-only.  */
        p = xmalloc (sizeof *p);
        *p = *type;
        p->next = image_types;
        image_types = p;
-       success = Qt;
      }
  
!   CACHE_IMAGE_TYPE (*type->type, success);
!   return success;
! }
! 
! 
! /* Look up image type SYMBOL, and return a pointer to its image_type
!    structure.  Value is null if SYMBOL is not a known image type.  */
! 
! static inline struct image_type *
! lookup_image_type (Lisp_Object symbol)
! {
!   struct image_type *type;
! 
!   /* We must initialize the image-type if it hasn't been already.  */
!   if (NILP (Finit_image_library (symbol, Vdynamic_library_alist)))
!     return 0;			/* unimplemented */
! 
!   for (type = image_types; type; type = type->next)
!     if (EQ (symbol, *type->type))
!       break;
! 
!   return type;
  }
  
  
--- 579,633 ----
    do { Vimage_types = Fcons (type, Vimage_types); } while (0)
  
  /* Define a new image type from TYPE.  This adds a copy of TYPE to
!    image_types and caches the loading status of TYPE.
  
!    LIBRARIES is an alist associating dynamic libraries to external
!    files implementing them, which is passed to the image library
!    initialization function if necessary.  A nil value defaults to
!    Vdynamic_library_alist.  */
! 
! static struct image_type *
! define_image_type (struct image_type *type, Lisp_Object libraries)
! {
!   struct image_type *p = NULL;
!   Lisp_Object target_type = *type->type;
!   int type_valid = 1;
  
!   BLOCK_INPUT;
! 
!   for (p = image_types; p; p = p->next)
!     if (EQ (*p->type, target_type))
!       goto done;
! 
!   if (type->init)
      {
! #ifdef HAVE_NTGUI
!       /* If we failed to load the library before, don't try again.  */
!       Lisp_Object tested = Fassq (target_type, Vlibrary_cache);
!       if (CONSP (tested) && NILP (XCDR (tested)))
! 	type_valid = 0;
!       else
! #endif
! 	{
! 	  /* If the load failed, avoid trying again.  */
! 	  type_valid = type->init (libraries);
! 	  CACHE_IMAGE_TYPE (target_type, type_valid ? Qt : Qnil);
! 	}
!     }
  
+   if (type_valid)
+     {
        /* Make a copy of TYPE to avoid a bus error in a dumped Emacs.
           The initialized data segment is read-only.  */
        p = xmalloc (sizeof *p);
        *p = *type;
        p->next = image_types;
        image_types = p;
      }
  
!  done:
!   UNBLOCK_INPUT;
!   return p;
  }
  
  
***************
*** 653,659 ****
  	    if (CONSP (tem) && SYMBOLP (XCAR (tem)))
  	      {
  		struct image_type *type;
! 		type = lookup_image_type (XCAR (tem));
  		if (type)
  		  valid_p = type->valid_p (object);
  	      }
--- 654,660 ----
  	    if (CONSP (tem) && SYMBOLP (XCAR (tem)))
  	      {
  		struct image_type *type;
! 		type = lookup_image_type (XCAR (tem), Qnil);
  		if (type)
  		  valid_p = type->valid_p (object);
  	      }
***************
*** 986,992 ****
  
    eassert (valid_image_p (spec));
    img->dependencies = NILP (file) ? Qnil : list1 (file);
!   img->type = lookup_image_type (image_spec_value (spec, QCtype, NULL));
    eassert (img->type != NULL);
    img->spec = spec;
    img->lisp_data = Qnil;
--- 987,993 ----
  
    eassert (valid_image_p (spec));
    img->dependencies = NILP (file) ? Qnil : list1 (file);
!   img->type = lookup_image_type (image_spec_value (spec, QCtype, NULL), Qnil);
    eassert (img->type != NULL);
    img->spec = spec;
    img->lisp_data = Qnil;
***************
*** 2262,2267 ****
--- 2263,2269 ----
    xbm_image_p,
    xbm_load,
    x_clear_image,
+   NULL,
    NULL
  };
  
***************
*** 3051,3056 ****
--- 3053,3064 ----
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_xpm_functions (Lisp_Object);
+ #else
+ #define init_xpm_functions NULL
+ #endif
+ 
  /* Structure describing the image type XPM.  */
  
  static struct image_type xpm_type =
***************
*** 3059,3064 ****
--- 3067,3073 ----
    xpm_image_p,
    xpm_load,
    x_clear_image,
+   init_xpm_functions,
    NULL
  };
  
***************
*** 4981,4986 ****
--- 4990,4996 ----
    pbm_image_p,
    pbm_load,
    x_clear_image,
+   NULL,
    NULL
  };
  
***************
*** 5387,5392 ****
--- 5397,5408 ----
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_png_functions (Lisp_Object);
+ #else
+ #define init_png_functions NULL
+ #endif
+ 
  /* Structure describing the image type `png'.  */
  
  static struct image_type png_type =
***************
*** 5395,5400 ****
--- 5411,5417 ----
    png_image_p,
    png_load,
    x_clear_image,
+   init_png_functions,
    NULL
  };
  
***************
*** 6039,6044 ****
--- 6056,6067 ----
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_jpeg_functions (Lisp_Object);
+ #else
+ #define init_jpeg_functions NULL
+ #endif
+ 
  /* Structure describing the image type `jpeg'.  */
  
  static struct image_type jpeg_type =
***************
*** 6047,6052 ****
--- 6070,6076 ----
    jpeg_image_p,
    jpeg_load,
    x_clear_image,
+   init_jpeg_functions,
    NULL
  };
  
***************
*** 6624,6629 ****
--- 6648,6659 ----
    {":index",		IMAGE_NON_NEGATIVE_INTEGER_VALUE,	0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_tiff_functions (Lisp_Object);
+ #else
+ #define init_tiff_functions NULL
+ #endif
+ 
  /* Structure describing the image type `tiff'.  */
  
  static struct image_type tiff_type =
***************
*** 6632,6637 ****
--- 6662,6668 ----
    tiff_image_p,
    tiff_load,
    x_clear_image,
+   init_tiff_functions,
    NULL
  };
  
***************
*** 7072,7077 ****
--- 7103,7114 ----
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_gif_functions (Lisp_Object);
+ #else
+ #define init_gif_functions NULL
+ #endif
+ 
  /* Structure describing the image type `gif'.  */
  
  static struct image_type gif_type =
***************
*** 7080,7085 ****
--- 7117,7123 ----
    gif_image_p,
    gif_load,
    gif_clear_image,
+   init_gif_functions,
    NULL
  };
  
***************
*** 7562,7567 ****
--- 7600,7611 ----
      {":crop",		IMAGE_DONT_CHECK_VALUE_TYPE,		0}
    };
  
+ #ifdef HAVE_NTGUI
+ static int init_imagemagick_functions (Lisp_Object);
+ #else
+ #define init_imagemagick_functions NULL
+ #endif
+ 
  /* Structure describing the image type for any image handled via
     ImageMagick.  */
  
***************
*** 7571,7576 ****
--- 7615,7621 ----
      imagemagick_image_p,
      imagemagick_load,
      imagemagick_clear_image,
+     init_imagemagick_functions,
      NULL
    };
  
***************
*** 8109,8130 ****
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
  /* Structure describing the image type `svg'.  Its the same type of
     structure defined for all image formats, handled by emacs image
     functions.  See struct image_type in dispextern.h.  */
  
  static struct image_type svg_type =
  {
-   /* An identifier showing that this is an image structure for the SVG format.  */
    &Qsvg,
-   /* Handle to a function that can be used to identify a SVG file.  */
    svg_image_p,
-   /* Handle to function used to load a SVG file.  */
    svg_load,
-   /* Handle to function to free sresources for SVG.  */
    x_clear_image,
!   /* An internal field to link to the next image type in a list of
!      image types, will be filled in when registering the format.  */
    NULL
  };
  
--- 8154,8176 ----
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_svg_functions (Lisp_Object);
+ #else
+ #define init_svg_functions NULL
+ #endif
+ 
  /* Structure describing the image type `svg'.  Its the same type of
     structure defined for all image formats, handled by emacs image
     functions.  See struct image_type in dispextern.h.  */
  
  static struct image_type svg_type =
  {
    &Qsvg,
    svg_image_p,
    svg_load,
    x_clear_image,
!   init_svg_functions,
    NULL
  };
  
***************
*** 8504,8509 ****
--- 8550,8561 ----
    {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
  };
  
+ #ifdef HAVE_NTGUI
+ static int init_gs_functions (Lisp_Object);
+ #else
+ #define init_gs_functions NULL
+ #endif
+ 
  /* Structure describing the image type `ghostscript'.  */
  
  static struct image_type gs_type =
***************
*** 8512,8517 ****
--- 8564,8570 ----
    gs_image_p,
    gs_load,
    gs_clear_image,
+   init_gs_functions,
    NULL
  };
  
***************
*** 8774,8789 ****
  			    Initialization
   ***********************************************************************/
  
- #ifdef HAVE_NTGUI
- /* Image types that rely on external libraries are loaded dynamically
-    if the library is available.  */
- #define CHECK_LIB_AVAILABLE(image_type, init_lib_fn, libraries) \
-   define_image_type (image_type, init_lib_fn (libraries))
- #else
- #define CHECK_LIB_AVAILABLE(image_type, init_lib_fn, libraries) \
-   define_image_type (image_type, 1)
- #endif /* HAVE_NTGUI */
- 
  DEFUN ("init-image-library", Finit_image_library, Sinit_image_library, 2, 2, 0,
         doc: /* Initialize image library implementing image type TYPE.
  Return non-nil if TYPE is a supported image type.
--- 8827,8832 ----
***************
*** 8793,8853 ****
  of `dynamic-library-alist', which see).  */)
    (Lisp_Object type, Lisp_Object libraries)
  {
! #ifdef HAVE_NTGUI
!   /* Don't try to reload the library.  */
!   Lisp_Object tested = Fassq (type, Vlibrary_cache);
!   if (CONSP (tested))
!     return XCDR (tested);
! #endif
  
    /* Types pbm and xbm are built-in and always available.  */
!   if (EQ (type, Qpbm) || EQ (type, Qxbm))
!     return Qt;
  
  #if defined (HAVE_XPM) || defined (HAVE_NS)
    if (EQ (type, Qxpm))
!     return CHECK_LIB_AVAILABLE (&xpm_type, init_xpm_functions, libraries);
  #endif
  
  #if defined (HAVE_JPEG) || defined (HAVE_NS)
    if (EQ (type, Qjpeg))
!     return CHECK_LIB_AVAILABLE (&jpeg_type, init_jpeg_functions, libraries);
  #endif
  
  #if defined (HAVE_TIFF) || defined (HAVE_NS)
    if (EQ (type, Qtiff))
!     return CHECK_LIB_AVAILABLE (&tiff_type, init_tiff_functions, libraries);
  #endif
  
  #if defined (HAVE_GIF) || defined (HAVE_NS)
    if (EQ (type, Qgif))
!     return CHECK_LIB_AVAILABLE (&gif_type, init_gif_functions, libraries);
  #endif
  
  #if defined (HAVE_PNG) || defined (HAVE_NS)
    if (EQ (type, Qpng))
!     return CHECK_LIB_AVAILABLE (&png_type, init_png_functions, libraries);
  #endif
  
  #if defined (HAVE_RSVG)
    if (EQ (type, Qsvg))
!     return CHECK_LIB_AVAILABLE (&svg_type, init_svg_functions, libraries);
  #endif
  
  #if defined (HAVE_IMAGEMAGICK)
    if (EQ (type, Qimagemagick))
!     return CHECK_LIB_AVAILABLE (&imagemagick_type, init_imagemagick_functions,
!                                 libraries);
  #endif
  
  #ifdef HAVE_GHOSTSCRIPT
    if (EQ (type, Qpostscript))
!     return CHECK_LIB_AVAILABLE (&gs_type, init_gs_functions, libraries);
  #endif
  
!   /* If the type is not recognized, avoid testing it ever again.  */
!   CACHE_IMAGE_TYPE (type, Qnil);
!   return Qnil;
  }
  
  void
--- 8836,8906 ----
  of `dynamic-library-alist', which see).  */)
    (Lisp_Object type, Lisp_Object libraries)
  {
!   return lookup_image_type (type, libraries) ? Qt : Qnil;
! }
! 
! /* Look up image type TYPE, and return a pointer to its image_type
!    structure.  Return 0 if TYPE is not a known image type.
! 
!    LIBRARIES is an alist associating dynamic libraries to external
!    files implementing them, which is passed to the image library
!    initialization function if necessary.  A nil value defaults to
!    Vdynamic_library_alist.  */
! 
! static struct image_type *
! lookup_image_type (Lisp_Object type, Lisp_Object libraries)
! {
!   if (NILP (libraries))
!     libraries = Vdynamic_library_alist;
  
    /* Types pbm and xbm are built-in and always available.  */
!   if (EQ (type, Qpbm))
!     return define_image_type (&pbm_type, libraries);
! 
!   if (EQ (type, Qxbm))
!     return define_image_type (&xbm_type, libraries);
  
  #if defined (HAVE_XPM) || defined (HAVE_NS)
    if (EQ (type, Qxpm))
!     return define_image_type (&xpm_type, libraries);
  #endif
  
  #if defined (HAVE_JPEG) || defined (HAVE_NS)
    if (EQ (type, Qjpeg))
!     return define_image_type (&jpeg_type, libraries);
  #endif
  
  #if defined (HAVE_TIFF) || defined (HAVE_NS)
    if (EQ (type, Qtiff))
!     return define_image_type (&tiff_type, libraries);
  #endif
  
  #if defined (HAVE_GIF) || defined (HAVE_NS)
    if (EQ (type, Qgif))
!     return define_image_type (&gif_type, libraries);
  #endif
  
  #if defined (HAVE_PNG) || defined (HAVE_NS)
    if (EQ (type, Qpng))
!     return define_image_type (&png_type, libraries);
  #endif
  
  #if defined (HAVE_RSVG)
    if (EQ (type, Qsvg))
!     return define_image_type (&svg_type, libraries);
  #endif
  
  #if defined (HAVE_IMAGEMAGICK)
    if (EQ (type, Qimagemagick))
!     return define_image_type (&imagemagick_type, libraries);
  #endif
  
  #ifdef HAVE_GHOSTSCRIPT
    if (EQ (type, Qpostscript))
!     return define_image_type (&gs_type, libraries);
  #endif
  
!   return NULL;
  }
  
  void
***************
*** 8884,8892 ****
    DEFSYM (Qxbm, "xbm");
    ADD_IMAGE_TYPE (Qxbm);
  
-   define_image_type (&xbm_type, 1);
-   define_image_type (&pbm_type, 1);
- 
    DEFSYM (Qcount, "count");
    DEFSYM (Qextension_data, "extension-data");
    DEFSYM (Qdelay, "delay");
--- 8937,8942 ----

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: lekktu@gmail.com, cyd@gnu.org, jwalt@garni.ch, 12463@debbugs.gnu.org
> Date: Fri, 21 Sep 2012 16:34:41 -0400
> 
> >> Sorry, can't remember.  Probably because I generally prefer passing
> >> arguments explicitly over passing them via dynamic binding.
> > We have gobs of functions in Emacs that access global variables like
> > Vdynamic_library_alist.  I don't think one more function will change
> > anything.
> 
> The question is not "global or not" but "let-bound or not".
> If it's generally accessed as a global var, that's perfectly fine.
> If it's generally let-bound, I prefer an explicit argument.

Sorry, I don't follow.  We are discussing C code, so how do you mean
"let-bound"?  If you mean by using specbind, then no, it isn't
let-bound, it's a global variable that always has the same value as it
had when the last cons cell was added to it.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Cc: Juanma Barranquero <lekktu@gmail.com>,  jwalt@garni.ch,  12463@debbugs.gnu.org
> Date: Sat, 22 Sep 2012 09:23:37 +0800
> 
> ! #ifdef HAVE_NTGUI
> !       /* If we failed to load the library before, don't try again.  */
> !       Lisp_Object tested = Fassq (target_type, Vlibrary_cache);
> !       if (CONSP (tested) && NILP (XCDR (tested)))
> ! 	type_valid = 0;
> !       else
> ! #endif
> ! 	{
> ! 	  /* If the load failed, avoid trying again.  */
> ! 	  type_valid = type->init (libraries);
> ! 	  CACHE_IMAGE_TYPE (target_type, type_valid ? Qt : Qnil);
> ! 	}
> !     }

The last comment seems to be redundant, given the one before it.

> ***************
> *** 8884,8892 ****
>     DEFSYM (Qxbm, "xbm");
>     ADD_IMAGE_TYPE (Qxbm);
>   
> -   define_image_type (&xbm_type, 1);
> -   define_image_type (&pbm_type, 1);

Why are these two lines being removed, while the corresponding
ADD_IMAGE_TYPE lines are not?

Thanks.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

>> ***************
>> *** 8884,8892 ****
>>     DEFSYM (Qxbm, "xbm");
>>     ADD_IMAGE_TYPE (Qxbm);
>>   
>> -   define_image_type (&xbm_type, 1);
>> -   define_image_type (&pbm_type, 1);
>
> Why are these two lines being removed, while the corresponding
> ADD_IMAGE_TYPE lines are not?

This change moves define_image_type into lookup_image_type, so that
these two image types are handled just like the other image types, for
simplicity.  The ADD_IMAGE_TYPE stuff is also done for the non xmb/pbm
image types.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Cc: lekktu@gmail.com,  jwalt@garni.ch,  12463@debbugs.gnu.org
> Date: Sat, 22 Sep 2012 19:05:51 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> ***************
> >> *** 8884,8892 ****
> >>     DEFSYM (Qxbm, "xbm");
> >>     ADD_IMAGE_TYPE (Qxbm);
> >>   
> >> -   define_image_type (&xbm_type, 1);
> >> -   define_image_type (&pbm_type, 1);
> >
> > Why are these two lines being removed, while the corresponding
> > ADD_IMAGE_TYPE lines are not?
> 
> This change moves define_image_type into lookup_image_type, so that
> these two image types are handled just like the other image types, for
> simplicity.

Right, I missed that.  Sorry.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Chong Yidong]

I've committed the patch to trunk.  Please let me know if this causes
any problems on Windows.  Juanma, you can go ahead with your work on
generalizing Vlibrary_cache, and thanks for waiting.

I left the LIBRARIES issue alone in this patch, but I do think we should
eliminate the second arg to init-image-library.  I don't see what value
other than `dynamic-library-alist' could possibly be supplied for this
argument.  Any objections to that?

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Cc: lekktu@gmail.com,  jwalt@garni.ch,  12463@debbugs.gnu.org
> Date: Sat, 22 Sep 2012 22:14:47 +0800
> 
> I've committed the patch to trunk.

Thanks.

> Please let me know if this causes any problems on Windows.

None that I could spot.

> I left the LIBRARIES issue alone in this patch, but I do think we should
> eliminate the second arg to init-image-library.  I don't see what value
> other than `dynamic-library-alist' could possibly be supplied for this
> argument.  Any objections to that?

Not from me.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Juanma Barranquero]

On Sat, Sep 22, 2012 at 4:14 PM, Chong Yidong <cyd@gnu.org> wrote:

> Juanma, you can go ahead with your work on
> generalizing Vlibrary_cache, and thanks for waiting.

Well, Vlibrary_cache is already pretty general. I only want to move it
to emacs.c.

Eli and I had talked about forcing the ops that work on Vlibrary_cache
and image_types to not cache anything before dumping, but now that the
entries for xbm and pbm are added the same way that other image
libraries, I wonder whether wouldn't be easier/cleaner to simply set
Vlibrary_cache = Qnil and image_types = NULL just before dumping.

> I left the LIBRARIES issue alone in this patch, but I do think we should
> eliminate the second arg to init-image-library.  I don't see what value
> other than `dynamic-library-alist' could possibly be supplied for this
> argument.  Any objections to that?

Please, go for it.

    Juanma

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Sat, 22 Sep 2012 21:20:48 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, jwalt@garni.ch, 12463@debbugs.gnu.org
> 
> Eli and I had talked about forcing the ops that work on Vlibrary_cache
> and image_types to not cache anything before dumping, but now that the
> entries for xbm and pbm are added the same way that other image
> libraries, I wonder whether wouldn't be easier/cleaner to simply set
> Vlibrary_cache = Qnil and image_types = NULL just before dumping.

I think that would leak memory.  Not a catastrophe, but still not
clean.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Juanma Barranquero]

On Sat, Sep 22, 2012 at 9:46 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> I think that would leak memory.

Because of image_types? Can't the memory be xfree'd?

    Juanma

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Stefan Monnier]

>> >> Sorry, can't remember.  Probably because I generally prefer passing
>> >> arguments explicitly over passing them via dynamic binding.
>> > We have gobs of functions in Emacs that access global variables like
>> > Vdynamic_library_alist.  I don't think one more function will change
>> > anything.
>> The question is not "global or not" but "let-bound or not".
>> If it's generally accessed as a global var, that's perfectly fine.
>> If it's generally let-bound, I prefer an explicit argument.
> Sorry, I don't follow.  We are discussing C code, so how do you mean
> "let-bound"?  If you mean by using specbind, then no, it isn't
> let-bound, it's a global variable that always has the same value as it
> had when the last cons cell was added to it.

I'm saying: is the variable usually set once and left alone, or is it
usually let-bound around the call to the function that uses
the variable?

In the case of dynamic-library-alist it seems it's never let-bound, so
it's probably better to access it directly as a global var, rather than
pass it as a parameter.


        Stefan

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Juanma Barranquero]

On Sat, Sep 22, 2012 at 10:20 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:

> In the case of dynamic-library-alist it seems it's never let-bound, so
> it's probably better to access it directly as a global var, rather than
> pass it as a parameter.

I'd be very surprised to find that someone ever let-bound
dynamic-library-alist. I'd bet that anyone who ever needed to use a
non-standard value (assuming someone did) just assigned it on .emacs.

    Juanma

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Sat, 22 Sep 2012 21:53:23 +0200
> Cc: cyd@gnu.org, jwalt@garni.ch, 12463@debbugs.gnu.org
> 
> On Sat, Sep 22, 2012 at 9:46 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > I think that would leak memory.
> 
> Because of image_types? Can't the memory be xfree'd?

Yes, you can, if you put some code to do that before setting it to
NULL.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Eli Zaretskii]

> I'm saying: is the variable usually set once and left alone, or is it
> usually let-bound around the call to the function that uses
> the variable?
> 
> In the case of dynamic-library-alist it seems it's never let-bound, so
> it's probably better to access it directly as a global var, rather than
> pass it as a parameter.

It is never let-bound.  It is modified by adding cons cells to it, and
used by accessing those cells.

bug#12463: 24.2; pos-visible-in-window-p gets slower over time

[Chong Yidong]

Juanma Barranquero <lekktu@gmail.com> writes:

> On Sat, Sep 22, 2012 at 10:20 PM, Stefan Monnier
> <monnier@iro.umontreal.ca> wrote:
>
>> In the case of dynamic-library-alist it seems it's never let-bound, so
>> it's probably better to access it directly as a global var, rather than
>> pass it as a parameter.
>
> I'd be very surprised to find that someone ever let-bound
> dynamic-library-alist. I'd bet that anyone who ever needed to use a
> non-standard value (assuming someone did) just assigned it on .emacs.

OK, I've removed the LIBRARIES arguments, so that the dynamic loading
code always uses dynamic-library-alist.