* bug#12463: 24.2; pos-visible-in-window-p gets slower over time
@ 2012-09-17 23:51 Jörg Walter
2012-09-18 7:46 ` Eli Zaretskii
0 siblings, 1 reply; 50+ messages in thread
From: Jörg Walter @ 2012-09-17 23:51 UTC (permalink / raw)
To: 12463
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
^ permalink raw reply [flat|nested] 50+ messages in thread* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-17 23:51 bug#12463: 24.2; pos-visible-in-window-p gets slower over time Jörg Walter @ 2012-09-18 7:46 ` Eli Zaretskii 2012-09-18 9:46 ` Jörg Walter 2012-09-18 15:17 ` Chong Yidong 0 siblings, 2 replies; 50+ messages in thread From: Eli Zaretskii @ 2012-09-18 7:46 UTC (permalink / raw) To: Jörg Walter; +Cc: 12463 > 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? ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 7:46 ` Eli Zaretskii @ 2012-09-18 9:46 ` Jörg Walter 2012-09-18 10:23 ` Eli Zaretskii 2012-09-18 15:05 ` Matt Lundin 2012-09-18 15:17 ` Chong Yidong 1 sibling, 2 replies; 50+ messages in thread From: Jörg Walter @ 2012-09-18 9:46 UTC (permalink / raw) To: 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 9:46 ` Jörg Walter @ 2012-09-18 10:23 ` Eli Zaretskii 2012-09-18 15:05 ` Matt Lundin 1 sibling, 0 replies; 50+ messages in thread From: Eli Zaretskii @ 2012-09-18 10:23 UTC (permalink / raw) To: Jörg Walter; +Cc: 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 9:46 ` Jörg Walter 2012-09-18 10:23 ` Eli Zaretskii @ 2012-09-18 15:05 ` Matt Lundin 2012-09-18 16:24 ` Eli Zaretskii 1 sibling, 1 reply; 50+ messages in thread From: Matt Lundin @ 2012-09-18 15:05 UTC (permalink / raw) To: Jörg Walter; +Cc: 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 15:05 ` Matt Lundin @ 2012-09-18 16:24 ` Eli Zaretskii 0 siblings, 0 replies; 50+ messages in thread From: Eli Zaretskii @ 2012-09-18 16:24 UTC (permalink / raw) To: Matt Lundin; +Cc: jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 7:46 ` Eli Zaretskii 2012-09-18 9:46 ` Jörg Walter @ 2012-09-18 15:17 ` Chong Yidong 2012-09-18 16:18 ` Jörg Walter 2012-09-18 16:19 ` Eli Zaretskii 1 sibling, 2 replies; 50+ messages in thread From: Chong Yidong @ 2012-09-18 15:17 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Jörg Walter, 12463 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 15:17 ` Chong Yidong @ 2012-09-18 16:18 ` Jörg Walter 2012-09-20 23:22 ` Juanma Barranquero 2012-09-18 16:19 ` Eli Zaretskii 1 sibling, 1 reply; 50+ messages in thread From: Jörg Walter @ 2012-09-18 16:18 UTC (permalink / raw) To: 12463 [-- 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); ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 16:18 ` Jörg Walter @ 2012-09-20 23:22 ` Juanma Barranquero 2012-09-21 3:52 ` Chong Yidong 0 siblings, 1 reply; 50+ messages in thread From: Juanma Barranquero @ 2012-09-20 23:22 UTC (permalink / raw) To: Jörg Walter; +Cc: 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-20 23:22 ` Juanma Barranquero @ 2012-09-21 3:52 ` Chong Yidong 2012-09-21 5:42 ` Chong Yidong ` (2 more replies) 0 siblings, 3 replies; 50+ messages in thread From: Chong Yidong @ 2012-09-21 3:52 UTC (permalink / raw) To: Juanma Barranquero; +Cc: Jörg Walter, 12463 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 3:52 ` Chong Yidong @ 2012-09-21 5:42 ` Chong Yidong 2012-09-21 7:34 ` Eli Zaretskii 2012-09-21 9:10 ` Juanma Barranquero 2012-09-21 6:58 ` Eli Zaretskii 2012-09-21 8:36 ` Juanma Barranquero 2 siblings, 2 replies; 50+ messages in thread From: Chong Yidong @ 2012-09-21 5:42 UTC (permalink / raw) To: Juanma Barranquero; +Cc: Jörg Walter, 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 5:42 ` Chong Yidong @ 2012-09-21 7:34 ` Eli Zaretskii 2012-09-21 9:24 ` Chong Yidong 2012-09-21 9:10 ` Juanma Barranquero 1 sibling, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-21 7:34 UTC (permalink / raw) To: Chong Yidong; +Cc: lekktu, jwalt, 12463 > 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? ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 7:34 ` Eli Zaretskii @ 2012-09-21 9:24 ` Chong Yidong 2012-09-21 10:47 ` Juanma Barranquero 0 siblings, 1 reply; 50+ messages in thread From: Chong Yidong @ 2012-09-21 9:24 UTC (permalink / raw) To: Eli Zaretskii; +Cc: lekktu, jwalt, 12463 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 9:24 ` Chong Yidong @ 2012-09-21 10:47 ` Juanma Barranquero 2012-09-21 12:33 ` Eli Zaretskii 0 siblings, 1 reply; 50+ messages in thread From: Juanma Barranquero @ 2012-09-21 10:47 UTC (permalink / raw) To: Chong Yidong; +Cc: jwalt, 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 10:47 ` Juanma Barranquero @ 2012-09-21 12:33 ` Eli Zaretskii 2012-09-21 16:38 ` Stefan Monnier 0 siblings, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-21 12:33 UTC (permalink / raw) To: Juanma Barranquero, Stefan Monnier; +Cc: cyd, jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 12:33 ` Eli Zaretskii @ 2012-09-21 16:38 ` Stefan Monnier 2012-09-21 16:58 ` Eli Zaretskii 0 siblings, 1 reply; 50+ messages in thread From: Stefan Monnier @ 2012-09-21 16:38 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Juanma Barranquero, cyd, jwalt, 12463 >> > 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 16:38 ` Stefan Monnier @ 2012-09-21 16:58 ` Eli Zaretskii 2012-09-21 20:34 ` Stefan Monnier 0 siblings, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-21 16:58 UTC (permalink / raw) To: Stefan Monnier; +Cc: lekktu, cyd, jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 16:58 ` Eli Zaretskii @ 2012-09-21 20:34 ` Stefan Monnier 2012-09-22 6:58 ` Eli Zaretskii 0 siblings, 1 reply; 50+ messages in thread From: Stefan Monnier @ 2012-09-21 20:34 UTC (permalink / raw) To: Eli Zaretskii; +Cc: lekktu, cyd, jwalt, 12463 >> 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 20:34 ` Stefan Monnier @ 2012-09-22 6:58 ` Eli Zaretskii 2012-09-22 20:20 ` Stefan Monnier 0 siblings, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-22 6:58 UTC (permalink / raw) To: Stefan Monnier; +Cc: lekktu, cyd, jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 6:58 ` Eli Zaretskii @ 2012-09-22 20:20 ` Stefan Monnier 2012-09-22 20:31 ` Juanma Barranquero 2012-09-23 3:50 ` Eli Zaretskii 0 siblings, 2 replies; 50+ messages in thread From: Stefan Monnier @ 2012-09-22 20:20 UTC (permalink / raw) To: Eli Zaretskii; +Cc: lekktu, cyd, jwalt, 12463 >> >> 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 20:20 ` Stefan Monnier @ 2012-09-22 20:31 ` Juanma Barranquero 2012-09-23 9:17 ` Chong Yidong 2012-09-23 3:50 ` Eli Zaretskii 1 sibling, 1 reply; 50+ messages in thread From: Juanma Barranquero @ 2012-09-22 20:31 UTC (permalink / raw) To: Stefan Monnier; +Cc: cyd, jwalt, 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 20:31 ` Juanma Barranquero @ 2012-09-23 9:17 ` Chong Yidong 0 siblings, 0 replies; 50+ messages in thread From: Chong Yidong @ 2012-09-23 9:17 UTC (permalink / raw) To: Juanma Barranquero; +Cc: jwalt, 12463 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 20:20 ` Stefan Monnier 2012-09-22 20:31 ` Juanma Barranquero @ 2012-09-23 3:50 ` Eli Zaretskii 1 sibling, 0 replies; 50+ messages in thread From: Eli Zaretskii @ 2012-09-23 3:50 UTC (permalink / raw) To: Stefan Monnier; +Cc: lekktu, cyd, jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 5:42 ` Chong Yidong 2012-09-21 7:34 ` Eli Zaretskii @ 2012-09-21 9:10 ` Juanma Barranquero 2012-09-21 10:01 ` Chong Yidong 1 sibling, 1 reply; 50+ messages in thread From: Juanma Barranquero @ 2012-09-21 9:10 UTC (permalink / raw) To: Chong Yidong; +Cc: Jörg Walter, 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 9:10 ` Juanma Barranquero @ 2012-09-21 10:01 ` Chong Yidong 2012-09-21 17:03 ` Eli Zaretskii 0 siblings, 1 reply; 50+ messages in thread From: Chong Yidong @ 2012-09-21 10:01 UTC (permalink / raw) To: Juanma Barranquero; +Cc: Jörg Walter, 12463 [-- 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 10:01 ` Chong Yidong @ 2012-09-21 17:03 ` Eli Zaretskii 2012-09-21 17:07 ` Juanma Barranquero 0 siblings, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-21 17:03 UTC (permalink / raw) To: Chong Yidong; +Cc: lekktu, jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 17:03 ` Eli Zaretskii @ 2012-09-21 17:07 ` Juanma Barranquero 2012-09-21 17:45 ` Eli Zaretskii 0 siblings, 1 reply; 50+ messages in thread From: Juanma Barranquero @ 2012-09-21 17:07 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Chong Yidong, jwalt, 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 17:07 ` Juanma Barranquero @ 2012-09-21 17:45 ` Eli Zaretskii 2012-09-22 1:23 ` Chong Yidong 0 siblings, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-21 17:45 UTC (permalink / raw) To: Juanma Barranquero; +Cc: cyd, jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 17:45 ` Eli Zaretskii @ 2012-09-22 1:23 ` Chong Yidong 2012-09-22 8:36 ` Eli Zaretskii 0 siblings, 1 reply; 50+ messages in thread From: Chong Yidong @ 2012-09-22 1:23 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Juanma Barranquero, jwalt, 12463 [-- 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 ---- ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 1:23 ` Chong Yidong @ 2012-09-22 8:36 ` Eli Zaretskii 2012-09-22 11:05 ` Chong Yidong 0 siblings, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-22 8:36 UTC (permalink / raw) To: Chong Yidong; +Cc: lekktu, jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 8:36 ` Eli Zaretskii @ 2012-09-22 11:05 ` Chong Yidong 2012-09-22 11:18 ` Eli Zaretskii 0 siblings, 1 reply; 50+ messages in thread From: Chong Yidong @ 2012-09-22 11:05 UTC (permalink / raw) To: Eli Zaretskii; +Cc: lekktu, jwalt, 12463 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 11:05 ` Chong Yidong @ 2012-09-22 11:18 ` Eli Zaretskii 2012-09-22 14:14 ` Chong Yidong 0 siblings, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-22 11:18 UTC (permalink / raw) To: Chong Yidong; +Cc: lekktu, jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 11:18 ` Eli Zaretskii @ 2012-09-22 14:14 ` Chong Yidong 2012-09-22 14:25 ` Eli Zaretskii 2012-09-22 19:20 ` Juanma Barranquero 0 siblings, 2 replies; 50+ messages in thread From: Chong Yidong @ 2012-09-22 14:14 UTC (permalink / raw) To: Eli Zaretskii; +Cc: lekktu, jwalt, 12463 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? ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 14:14 ` Chong Yidong @ 2012-09-22 14:25 ` Eli Zaretskii 2012-09-22 19:20 ` Juanma Barranquero 1 sibling, 0 replies; 50+ messages in thread From: Eli Zaretskii @ 2012-09-22 14:25 UTC (permalink / raw) To: Chong Yidong; +Cc: lekktu, jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 14:14 ` Chong Yidong 2012-09-22 14:25 ` Eli Zaretskii @ 2012-09-22 19:20 ` Juanma Barranquero 2012-09-22 19:46 ` Eli Zaretskii 1 sibling, 1 reply; 50+ messages in thread From: Juanma Barranquero @ 2012-09-22 19:20 UTC (permalink / raw) To: Chong Yidong; +Cc: jwalt, 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 19:20 ` Juanma Barranquero @ 2012-09-22 19:46 ` Eli Zaretskii 2012-09-22 19:53 ` Juanma Barranquero 0 siblings, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-22 19:46 UTC (permalink / raw) To: Juanma Barranquero; +Cc: cyd, jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 19:46 ` Eli Zaretskii @ 2012-09-22 19:53 ` Juanma Barranquero 2012-09-23 3:48 ` Eli Zaretskii 0 siblings, 1 reply; 50+ messages in thread From: Juanma Barranquero @ 2012-09-22 19:53 UTC (permalink / raw) To: Eli Zaretskii; +Cc: cyd, jwalt, 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-22 19:53 ` Juanma Barranquero @ 2012-09-23 3:48 ` Eli Zaretskii 0 siblings, 0 replies; 50+ messages in thread From: Eli Zaretskii @ 2012-09-23 3:48 UTC (permalink / raw) To: Juanma Barranquero; +Cc: cyd, jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 3:52 ` Chong Yidong 2012-09-21 5:42 ` Chong Yidong @ 2012-09-21 6:58 ` Eli Zaretskii 2012-09-21 8:36 ` Juanma Barranquero 2 siblings, 0 replies; 50+ messages in thread From: Eli Zaretskii @ 2012-09-21 6:58 UTC (permalink / raw) To: Chong Yidong; +Cc: lekktu, jwalt, 12463 > 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? ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 3:52 ` Chong Yidong 2012-09-21 5:42 ` Chong Yidong 2012-09-21 6:58 ` Eli Zaretskii @ 2012-09-21 8:36 ` Juanma Barranquero 2012-09-21 9:11 ` Chong Yidong 2 siblings, 1 reply; 50+ messages in thread From: Juanma Barranquero @ 2012-09-21 8:36 UTC (permalink / raw) To: Chong Yidong; +Cc: Jörg Walter, 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 8:36 ` Juanma Barranquero @ 2012-09-21 9:11 ` Chong Yidong 2012-09-21 9:17 ` Juanma Barranquero 0 siblings, 1 reply; 50+ messages in thread From: Chong Yidong @ 2012-09-21 9:11 UTC (permalink / raw) To: Juanma Barranquero; +Cc: Jörg Walter, 12463 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). ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-21 9:11 ` Chong Yidong @ 2012-09-21 9:17 ` Juanma Barranquero 0 siblings, 0 replies; 50+ messages in thread From: Juanma Barranquero @ 2012-09-21 9:17 UTC (permalink / raw) To: Chong Yidong; +Cc: Jörg Walter, 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 15:17 ` Chong Yidong 2012-09-18 16:18 ` Jörg Walter @ 2012-09-18 16:19 ` Eli Zaretskii 2012-09-18 16:26 ` Jörg Walter 1 sibling, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-18 16:19 UTC (permalink / raw) To: Chong Yidong; +Cc: jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 16:19 ` Eli Zaretskii @ 2012-09-18 16:26 ` Jörg Walter 2012-09-18 17:19 ` Eli Zaretskii 0 siblings, 1 reply; 50+ messages in thread From: Jörg Walter @ 2012-09-18 16:26 UTC (permalink / raw) To: 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 16:26 ` Jörg Walter @ 2012-09-18 17:19 ` Eli Zaretskii 2012-09-18 17:31 ` Juanma Barranquero 0 siblings, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-18 17:19 UTC (permalink / raw) To: Jörg Walter, Juanma Barranquero; +Cc: 12463 > 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? ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 17:19 ` Eli Zaretskii @ 2012-09-18 17:31 ` Juanma Barranquero 2012-09-18 20:00 ` Eli Zaretskii 0 siblings, 1 reply; 50+ messages in thread From: Juanma Barranquero @ 2012-09-18 17:31 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Jörg Walter, 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 17:31 ` Juanma Barranquero @ 2012-09-18 20:00 ` Eli Zaretskii 2012-09-19 2:31 ` Juanma Barranquero 0 siblings, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-18 20:00 UTC (permalink / raw) To: Juanma Barranquero; +Cc: jwalt, 12463 > 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. ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-18 20:00 ` Eli Zaretskii @ 2012-09-19 2:31 ` Juanma Barranquero 2012-09-19 2:57 ` Eli Zaretskii 0 siblings, 1 reply; 50+ messages in thread From: Juanma Barranquero @ 2012-09-19 2:31 UTC (permalink / raw) To: Eli Zaretskii; +Cc: jwalt, 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-19 2:31 ` Juanma Barranquero @ 2012-09-19 2:57 ` Eli Zaretskii 2012-09-19 3:03 ` Juanma Barranquero 0 siblings, 1 reply; 50+ messages in thread From: Eli Zaretskii @ 2012-09-19 2:57 UTC (permalink / raw) To: Juanma Barranquero; +Cc: jwalt, 12463 > 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? ^ permalink raw reply [flat|nested] 50+ messages in thread
* bug#12463: 24.2; pos-visible-in-window-p gets slower over time 2012-09-19 2:57 ` Eli Zaretskii @ 2012-09-19 3:03 ` Juanma Barranquero 0 siblings, 0 replies; 50+ messages in thread From: Juanma Barranquero @ 2012-09-19 3:03 UTC (permalink / raw) To: Eli Zaretskii; +Cc: jwalt, 12463 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2012-09-23 9:17 UTC | newest] Thread overview: 50+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-17 23:51 bug#12463: 24.2; pos-visible-in-window-p gets slower over time Jörg Walter 2012-09-18 7:46 ` Eli Zaretskii 2012-09-18 9:46 ` Jörg Walter 2012-09-18 10:23 ` Eli Zaretskii 2012-09-18 15:05 ` Matt Lundin 2012-09-18 16:24 ` Eli Zaretskii 2012-09-18 15:17 ` Chong Yidong 2012-09-18 16:18 ` Jörg Walter 2012-09-20 23:22 ` Juanma Barranquero 2012-09-21 3:52 ` Chong Yidong 2012-09-21 5:42 ` Chong Yidong 2012-09-21 7:34 ` Eli Zaretskii 2012-09-21 9:24 ` Chong Yidong 2012-09-21 10:47 ` Juanma Barranquero 2012-09-21 12:33 ` Eli Zaretskii 2012-09-21 16:38 ` Stefan Monnier 2012-09-21 16:58 ` Eli Zaretskii 2012-09-21 20:34 ` Stefan Monnier 2012-09-22 6:58 ` Eli Zaretskii 2012-09-22 20:20 ` Stefan Monnier 2012-09-22 20:31 ` Juanma Barranquero 2012-09-23 9:17 ` Chong Yidong 2012-09-23 3:50 ` Eli Zaretskii 2012-09-21 9:10 ` Juanma Barranquero 2012-09-21 10:01 ` Chong Yidong 2012-09-21 17:03 ` Eli Zaretskii 2012-09-21 17:07 ` Juanma Barranquero 2012-09-21 17:45 ` Eli Zaretskii 2012-09-22 1:23 ` Chong Yidong 2012-09-22 8:36 ` Eli Zaretskii 2012-09-22 11:05 ` Chong Yidong 2012-09-22 11:18 ` Eli Zaretskii 2012-09-22 14:14 ` Chong Yidong 2012-09-22 14:25 ` Eli Zaretskii 2012-09-22 19:20 ` Juanma Barranquero 2012-09-22 19:46 ` Eli Zaretskii 2012-09-22 19:53 ` Juanma Barranquero 2012-09-23 3:48 ` Eli Zaretskii 2012-09-21 6:58 ` Eli Zaretskii 2012-09-21 8:36 ` Juanma Barranquero 2012-09-21 9:11 ` Chong Yidong 2012-09-21 9:17 ` Juanma Barranquero 2012-09-18 16:19 ` Eli Zaretskii 2012-09-18 16:26 ` Jörg Walter 2012-09-18 17:19 ` Eli Zaretskii 2012-09-18 17:31 ` Juanma Barranquero 2012-09-18 20:00 ` Eli Zaretskii 2012-09-19 2:31 ` Juanma Barranquero 2012-09-19 2:57 ` Eli Zaretskii 2012-09-19 3:03 ` Juanma Barranquero
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).