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

[Chong Yidong <cyd@gnu.org>]

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