bug#68006: 30.0.50; Image-mode speed

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

I was looking into performance issue using image-mode when going from
one image to the next/previous (with 'image-next/previous-file') and I
found those lines in "image-mode.el":

--8<---------------cut here---------------start------------->8---
    ;; Discard any stale image data before looking it up again.
    (image-flush image)
    (setq image (append image (image-transform-properties image)))
    (setq props
	  `(display ,image
		    ;; intangible ,image
		    rear-nonsticky (display) ;; intangible
		    read-only t front-sticky (read-only)))
--8<---------------cut here---------------end--------------->8---

The call to 'image-flush' defeats the entire purpose of having an image
cache.  If I remove it, moving from previous/next is much more faster
(once this image is in the cache, of course).  Do you know why this code
is like that?

Best regards,


In GNU Emacs 30.0.50 (build 2, x86_64-unknown-openbsd7.4) of 2023-12-24
 built on computer
Repository revision: 13e46e2c1d33a3a48ecdcb56b745dbc53a4a3831
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101009
System Description: OpenBSD computer 7.4 GENERIC.MP#1549 amd64

Configured using:
 'configure CC=egcc CPPFLAGS=-I/usr/local/include
 LDFLAGS=-L/usr/local/lib MAKEINFO=gmakeinfo --prefix=/home/manuel/emacs
 --bindir=/home/manuel/bin --with-x-toolkit=no --without-cairo
 --without-dbus --without-gconf --without-gsettings --without-sound
 --without-compress-install'

Configured features:
FREETYPE GIF GLIB GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBOTF LIBXML2
MODULES NOTIFY KQUEUE OLDXMENU PDUMPER PNG RSVG SQLITE3 THREADS TIFF
TREE_SITTER WEBP X11 XDBE XFT XIM XINPUT2 XPM ZLIB

Important settings:
  value of $LC_CTYPE: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: ELisp/l

Minor modes in effect:
  bug-reference-prog-mode: t
  paredit-mode: t
  display-time-mode: t
  display-battery-mode: t
  desktop-save-mode: t
  server-mode: t
  override-global-mode: t
  repeat-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  minibuffer-regexp-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/manuel/.emacs.d/elpa/ef-themes-1.4.1/theme-loaddefs hides /home/manuel/emacs/share/emacs/30.0.50/lisp/theme-loaddefs

Features:
(shadow sort mail-extr emacsbug vc-annotate smerge-mode diff vc-bzr
vc-src vc-sccs vc-svn vc-cvs vc-rcs log-view pcvs-util whitespace
image-file image-converter org-agenda org-indent org-element org-persist
org-id avl-tree oc-basic ol-eww ol-rmail ol-mhe ol-irc ol-info ol-gnus
nnselect ol-docview doc-view image-mode exif ol-bibtex bibtex ol-bbdb
ol-w3m ol-doi org-link-doi gnus-icalendar org-capture org-refile org ob
ob-tangle ob-ref ob-lob ob-table ob-exp org-macro org-src ob-comint
org-pcomplete org-list org-footnote org-faces org-entities ob-emacs-lisp
ob-core ob-eval org-cycle org-table ol org-fold org-fold-core org-keys
oc org-loaddefs org-version org-compat org-macs autorevert filenotify
vc-hg sh-script smie treesit executable eww url-queue mm-url mule-util
jka-compr on-screen vc-dir ewoc vc vc-git diff-mode vc-dispatcher
bug-reference paredit gnus-dired time battery cus-load desktop frameset
exwm-randr xcb-randr exwm-config ido exwm exwm-input xcb-keysyms xcb-xkb
exwm-manage exwm-floating xcb-cursor xcb-render exwm-layout
exwm-workspace exwm-core xcb-ewmh xcb-icccm xcb xcb-xproto xcb-types
xcb-debug server modus-operandi-theme modus-themes zone speed-type
url-http url-auth url-gw nsm compat ytdious mingus libmpdee reporter
edebug debug backtrace transmission color calc-bin calc-ext calc
calc-loaddefs rect calc-macs supercite regi ebdb-message ebdb-gnus
gnus-msg gnus-art mm-uu mml2015 mm-view mml-smime smime gnutls dig
gnus-sum shr pixel-fill kinsoku url-file svg dom gnus-group gnus-undo
gnus-start gnus-dbus dbus xml gnus-cloud nnimap nnmail mail-source utf7
nnoo gnus-spec gnus-int gnus-range message sendmail yank-media puny
rfc822 mml mml-sec epa epg rfc6068 epg-config mm-decode mm-bodies
mm-encode mail-parse rfc2231 rfc2047 rfc2045 ietf-drums gmm-utils
mailheader gnus-win ebdb-mua ebdb-com crm ebdb-format ebdb mailabbrev
eieio-opt speedbar ezimage dframe find-func eieio-base timezone
icalendar gnus nnheader gnus-util mail-utils range mm-util mail-prsvr
wid-edit web-mode derived disp-table erlang-start skeleton cc-mode
cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars
cc-defs slime-asdf grep slime-tramp tramp rx trampver tramp-integration
files-x tramp-message tramp-compat xdg shell pcomplete parse-time
iso8601 time-date format-spec tramp-loaddefs slime-fancy
slime-indentation slime-cl-indent cl-indent slime-trace-dialog
slime-fontifying-fu slime-package-fu slime-references
slime-compiler-notes-tree advice slime-scratch slime-presentations
bridge slime-macrostep macrostep slime-mdot-fu slime-enclosing-context
slime-fuzzy slime-fancy-trace slime-fancy-inspector slime-c-p-c
slime-editing-commands slime-autodoc slime-repl slime-parse slime
apropos compile text-property-search etags fileloop generator xref
project arc-mode archive-mode noutline outline icons pp comint ansi-osc
ansi-color ring hyperspec thingatpt slime-autoloads edmacro kmacro
use-package-bind-key bind-key appt diary-lib diary-loaddefs cal-menu
calendar cal-loaddefs pcase dired-x dired-aux dired dired-loaddefs
cl-extra help-mode use-package-core repeat easy-mmode debbugs-autoloads
ebdb-autoloads ef-themes-autoloads exwm-autoloads hyperbole-autoloads
magit-autoloads git-commit-autoloads magit-section-autoloads
dash-autoloads on-screen-autoloads osm-autoloads paredit-autoloads
rust-mode-autoloads speed-type-autoloads transmission-autoloads
with-editor-autoloads info compat-autoloads ytdious-autoloads package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie generate-lisp-file url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs password-cache
json subr-x map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs
cl-lib rmc iso-transl tooltip cconv eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd touch-screen tool-bar dnd fontset image regexp-opt
fringe tabulated-list replace newcomment text-mode lisp-mode prog-mode
register page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads kqueue lcms2 dynamic-setting font-render-setting xinput2 x
multi-tty move-toolbar make-network-process emacs)

Memory information:
((conses 16 793954 635507) (symbols 48 52684 2)
 (strings 32 255659 44607) (string-bytes 1 7175025)
 (vectors 16 150516) (vector-slots 8 2617297 42627)
 (floats 8 567 3230) (intervals 56 20138 6985) (buffers 992 49))

-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> Date: Sun, 24 Dec 2023 17:44:37 +0100
> From:  Manuel Giraud via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I was looking into performance issue using image-mode when going from
> one image to the next/previous (with 'image-next/previous-file') and I
> found those lines in "image-mode.el":
> 
> --8<---------------cut here---------------start------------->8---
>     ;; Discard any stale image data before looking it up again.
>     (image-flush image)
>     (setq image (append image (image-transform-properties image)))
>     (setq props
> 	  `(display ,image
> 		    ;; intangible ,image
> 		    rear-nonsticky (display) ;; intangible
> 		    read-only t front-sticky (read-only)))
> --8<---------------cut here---------------end--------------->8---
> 
> The call to 'image-flush' defeats the entire purpose of having an image
> cache.  If I remove it, moving from previous/next is much more faster
> (once this image is in the cache, of course).  Do you know why this code
> is like that?

Look at its Git history, and look up discussions and bugs from those
times.

AFAIR, one problem is that images can change behind our backs.  But
maybe there are other reasons.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> The call to 'image-flush' defeats the entire purpose of having an image
>> cache.  If I remove it, moving from previous/next is much more faster
>> (once this image is in the cache, of course).  Do you know why this code
>> is like that?
>
> Look at its Git history, and look up discussions and bugs from those
> times.
>
> AFAIR, one problem is that images can change behind our backs.  But
> maybe there are other reasons.

So I trace this call back to dc255b24e30a (Chong Yidong, 2007-05-21).  At
that time 'image-flush' was called 'image-refresh' with the same
functionality of uncaching an image given its spec.

Before that, the whole image cache was cleared (fcfc4732e0d9, Chong
Yidong, 2006-02-09).  I did not find a bug reference and I still don't
understand why it is/was needed.
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 68006@debbugs.gnu.org
> Date: Mon, 25 Dec 2023 11:34:49 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Look at its Git history, and look up discussions and bugs from those
> > times.
> >
> > AFAIR, one problem is that images can change behind our backs.  But
> > maybe there are other reasons.
> 
> So I trace this call back to dc255b24e30a (Chong Yidong, 2007-05-21).  At
> that time 'image-flush' was called 'image-refresh' with the same
> functionality of uncaching an image given its spec.
> 
> Before that, the whole image cache was cleared (fcfc4732e0d9, Chong
> Yidong, 2006-02-09).  I did not find a bug reference and I still don't
> understand why it is/was needed.

I think the reason is what I mentioned above.

You seem to interpret the purpose of the image-cache differently from
its real purpose.  The real purpose of the image cache is not to hold
the image in memory for displaying it again and again, the purpose is
to allow the display engine to generate the pixels once, then reuse
those pixels during the current redisplay cycle or a few following
redisplay cycles.  That's why we can so easily evict images from the
cache when some time has passed: the cache is ephemeral anyway, and is
intended to serve only as a short-term memory.

When working with external image files, flushing the image cache is a
good measure to make sure we always display the file on disk, not some
previous copy of it.

If you have a specific application in mind where this cache must be
kept for longer times, we could perhaps arrange for that, but we will
then also need to make sure images are not evicted from the cache
during that time.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> I think the reason is what I mentioned above.
>
> You seem to interpret the purpose of the image-cache differently from
> its real purpose.  The real purpose of the image cache is not to hold
> the image in memory for displaying it again and again, the purpose is
> to allow the display engine to generate the pixels once, then reuse
> those pixels during the current redisplay cycle or a few following
> redisplay cycles.  That's why we can so easily evict images from the
> cache when some time has passed: the cache is ephemeral anyway, and is
> intended to serve only as a short-term memory.

Ok, I guess I understand this purpose...

> When working with external image files, flushing the image cache is a
> good measure to make sure we always display the file on disk, not some
> previous copy of it.

... but then this does not seem like a good fit for the usage in
image-mode.  When using 'image-next-file'/'image-previous-file', I
imagine that the idea is browse some pictures seamlessly.  Removing an
image from the cache each time seems to go against this.

About being sure to display the file on disk, maybe we could call
'image-flush' only if the file has changed since its display.  WDYT?
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 68006@debbugs.gnu.org
> Date: Mon, 25 Dec 2023 19:59:10 +0100
> 
> About being sure to display the file on disk, maybe we could call
> 'image-flush' only if the file has changed since its display.  WDYT?

Provided that the check is reliable, I guess so.

In any case, I think we should be cautious and leave a knob to get
back the old behavior, in case there are some situations we don't
anticipate that need to flush the caches.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: 68006@debbugs.gnu.org
>> Date: Mon, 25 Dec 2023 19:59:10 +0100
>> 
>> About being sure to display the file on disk, maybe we could call
>> 'image-flush' only if the file has changed since its display.  WDYT?
>
> Provided that the check is reliable, I guess so.
>
> In any case, I think we should be cautious and leave a knob to get
> back the old behavior, in case there are some situations we don't
> anticipate that need to flush the caches.

Hi,

What do you think of this?  Of course, it will need a NEWS entry but I
wanted to polish it first.  I have made it opt-in.  I have used it a bit
the cache could grow fast but I find it quite pleasant to use (for
Docview also).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-some-user-control-on-image-caching.patch --]
[-- Type: text/x-patch, Size: 3032 bytes --]

From 4f941c1fe7bc91b121f305b3a3239cf9a3944a6b Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Tue, 26 Dec 2023 15:35:17 +0100
Subject: [PATCH] Add some user control on image caching

* lisp/image-mode.el (image-eager-flush): New custom to control
image cache flush.
(image-toggle-display-image): Flush image upon on file disk
change.
* src/image.c (Fimage_timestamp): New function to access image
display timestamp.
---
 lisp/image-mode.el | 16 ++++++++++++++--
 src/image.c        | 25 +++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/lisp/image-mode.el b/lisp/image-mode.el
index d5ca6348c92..ccea96ecc26 100644
--- a/lisp/image-mode.el
+++ b/lisp/image-mode.el
@@ -93,6 +93,12 @@ image-auto-resize-on-window-resize
   :version "27.1"
   :group 'image)
 
+(defcustom image-eager-flush t
+  "Non-nil means flush image from cache eagerly."
+  :type 'boolean
+  :version "30.1"
+  :group 'image)
+
 (defvar-local image-transform-resize nil
   "The image resize operation.
 Non-nil to resize the image upon first display.
@@ -954,8 +960,14 @@ image-toggle-display-image
                   (plist-put (cdr image) :width
                              (plist-get (cdr image) :max-width)))))
 
-    ;; Discard any stale image data before looking it up again.
-    (image-flush image)
+    ;; Discard image data if its file has changed on disk.
+    (when (or image-eager-flush
+              (and filename
+                   (time-less-p
+                    (image-timestamp image)
+                    (file-attribute-modification-time
+                     (file-attributes filename)))))
+      (image-flush image))
     (setq image (append image (image-transform-properties image)))
     (setq props
 	  `(display ,image
diff --git a/src/image.c b/src/image.c
index 651ec0b34e5..3c2697d1e6f 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1650,6 +1650,30 @@ DEFUN ("image-metadata", Fimage_metadata, Simage_metadata, 1, 2, 0,
   return ext;
 }
 
+DEFUN ("image-timestamp", Fimage_timestamp, Simage_timestamp, 1, 2, 0,
+       doc: /* Return the time in seconds at which the image SPEC was
+last displayed.
+
+FRAME is the frame on which the image was displayed.  FRAME nil or
+omitted means use the selected frame.  */)
+  (Lisp_Object spec, Lisp_Object frame)
+{
+  Lisp_Object timestamp;
+
+  timestamp = Qnil;
+  if (valid_image_p (spec))
+    {
+      struct frame *f = decode_window_system_frame (frame);
+      ptrdiff_t id = lookup_image (f, spec, -1);
+      struct image *img = IMAGE_FROM_ID (f, id);
+
+      if (img)
+	timestamp = make_lisp_time (img->timestamp);
+    }
+
+  return timestamp;
+}
+
 \f
 /***********************************************************************
 		 Image type independent image structures
@@ -12908,6 +12932,7 @@ syms_of_image (void)
   defsubr (&Simage_size);
   defsubr (&Simage_mask_p);
   defsubr (&Simage_metadata);
+  defsubr (&Simage_timestamp);
   defsubr (&Simage_cache_size);
   defsubr (&Simagep);
 
-- 
2.43.0


[-- Attachment #3: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 68006@debbugs.gnu.org
> Date: Tue, 26 Dec 2023 15:45:11 +0100
> 
> >> About being sure to display the file on disk, maybe we could call
> >> 'image-flush' only if the file has changed since its display.  WDYT?
> >
> > Provided that the check is reliable, I guess so.
> >
> > In any case, I think we should be cautious and leave a knob to get
> > back the old behavior, in case there are some situations we don't
> > anticipate that need to flush the caches.
> 
> Hi,
> 
> What do you think of this?  Of course, it will need a NEWS entry but I
> wanted to polish it first.  I have made it opt-in.  I have used it a bit
> the cache could grow fast but I find it quite pleasant to use (for
> Docview also).

Using img->timestamp is not reliable enough, since that timestamp is
updated each time we call prepare_image_for_display, which can happen
many times during a session for the same image, and not necessarily
for actually displaying the image in a window.  For example, AFAICT if
you move across an image with C-n/C-p, we update the time stamp each
time vertical motion crosses the screen line with the image.  So I
think we'd need to store the file's time stamp or some other
signature.

The comparison with times-less-p is also risky: what if someone
replaces the image file with an older file?

I'd trust some kind of file checksum better, which we will have to
store alongside the image spec or as part of it.  (Don't we already do
something like that somewhere in image-*.el files?)

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> Using img->timestamp is not reliable enough, since that timestamp is
> updated each time we call prepare_image_for_display, which can happen
> many times during a session for the same image, and not necessarily
> for actually displaying the image in a window.  For example, AFAICT if
> you move across an image with C-n/C-p, we update the time stamp each
> time vertical motion crosses the screen line with the image.  So I
> think we'd need to store the file's time stamp or some other
> signature.

Right.

> The comparison with times-less-p is also risky: what if someone
> replaces the image file with an older file?

I don't understand what the risk is here?  It is just a cache.

> I'd trust some kind of file checksum better, which we will have to
> store alongside the image spec or as part of it.  (Don't we already do
> something like that somewhere in image-*.el files?)

For image-dired, there is 'image-dired-contents-sha1'.  Maybe this the
one you thought about?
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 68006@debbugs.gnu.org
> Date: Tue, 26 Dec 2023 19:07:00 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The comparison with times-less-p is also risky: what if someone
> > replaces the image file with an older file?
> 
> I don't understand what the risk is here?  It is just a cache.

The risk is that you won't notice the fact that the image file was
replaced.

> > I'd trust some kind of file checksum better, which we will have to
> > store alongside the image spec or as part of it.  (Don't we already do
> > something like that somewhere in image-*.el files?)
> 
> For image-dired, there is 'image-dired-contents-sha1'.  Maybe this the
> one you thought about?

Probably.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> For image-dired, there is 'image-dired-contents-sha1'.  Maybe this the
>> one you thought about?
>
> Probably.

Hi Eli,

I was comtemplating the idea of using 'image-dired-contents-sha1' to
ensure that the image contents has not changed on disk.  But then I
asked myself if a function "give me a unique id of this file based on
its content" does not already exist in Emacs.  I could not found one but
maybe we could create one (eg. file-unique-identifier)?

Or do you prefer to keep this into the image library?  In which case,
i'll move this function to "image.el" or "image-mode.el".
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 68006@debbugs.gnu.org
> Date: Wed, 27 Dec 2023 13:13:13 +0100
> 
> I was comtemplating the idea of using 'image-dired-contents-sha1' to
> ensure that the image contents has not changed on disk.  But then I
> asked myself if a function "give me a unique id of this file based on
> its content" does not already exist in Emacs.  I could not found one but
> maybe we could create one (eg. file-unique-identifier)?
> 
> Or do you prefer to keep this into the image library?  In which case,
> i'll move this function to "image.el" or "image-mode.el".

I think for now it should be kept in the image library, yes.  I don't
quite see why it would be needed for other files (if it was, we would
have had it already by now).

Thanks.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: 68006@debbugs.gnu.org
>> Date: Wed, 27 Dec 2023 13:13:13 +0100
>> 
>> I was comtemplating the idea of using 'image-dired-contents-sha1' to
>> ensure that the image contents has not changed on disk.  But then I
>> asked myself if a function "give me a unique id of this file based on
>> its content" does not already exist in Emacs.  I could not found one but
>> maybe we could create one (eg. file-unique-identifier)?
>> 
>> Or do you prefer to keep this into the image library?  In which case,
>> i'll move this function to "image.el" or "image-mode.el".
>
> I think for now it should be kept in the image library, yes.  I don't
> quite see why it would be needed for other files (if it was, we would
> have had it already by now).

Hi Eli,

What about this new patch?  It works for me as intended but might need
better names and better docs.  Thanks.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-some-user-control-in-image-mode-caching.patch --]
[-- Type: text/x-patch, Size: 4183 bytes --]

From c131c03dafedbdaa9035c95386a081c452f00934 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 29 Dec 2023 11:48:21 +0100
Subject: [PATCH] Add some user control in image-mode caching

* lisp/image-mode.el (image-mode-eager-cache-flush): New custom.
(image-toggle-display-image): Add a contents hash in image spec
when flushing cache lazily.

* lisp/image/image-dired-util.el (image-dired-thumb-name):
* lisp/image.el (image-contents-sha1): Move
'image-dired-contents-sha1' to 'image-contents-sha1' in toplevel
image library.
---
 lisp/image-mode.el             | 18 ++++++++++++++++--
 lisp/image.el                  | 11 ++++++++++-
 lisp/image/image-dired-util.el |  8 +-------
 3 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/lisp/image-mode.el b/lisp/image-mode.el
index d5ca6348c92..bad6624bc96 100644
--- a/lisp/image-mode.el
+++ b/lisp/image-mode.el
@@ -93,6 +93,14 @@ image-auto-resize-on-window-resize
   :version "27.1"
   :group 'image)
 
+(defcustom image-mode-eager-cache-flush t
+  "Non-nil means flush image from cache eagerly.
+When set to nil, be aware that the image cache could grow fast
+but an image previously displayed will show faster."
+  :type 'boolean
+  :version "30.1"
+  :group 'image)
+
 (defvar-local image-transform-resize nil
   "The image resize operation.
 Non-nil to resize the image upon first display.
@@ -954,8 +962,14 @@ image-toggle-display-image
                   (plist-put (cdr image) :width
                              (plist-get (cdr image) :max-width)))))
 
-    ;; Discard any stale image data before looking it up again.
-    (image-flush image)
+    (if image-mode-eager-cache-flush
+        ;; Discard any stale image data before looking it up again.
+        (image-flush image)
+      ;; Add a content based hash into image spec to be sure that the
+      ;; cache is updated should the on disk image change.
+      (when (and filename (file-exists-p filename))
+        (setq image (append image (list :hash (image-contents-sha1 filename))))))
+
     (setq image (append image (image-transform-properties image)))
     (setq props
 	  `(display ,image
diff --git a/lisp/image.el b/lisp/image.el
index e20fbcf4c98..072549aa88f 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -401,7 +401,16 @@ image-type-from-file-name
 	    ;; If nothing seems to be supported, return first type that matched.
 	    (or first (setq first type))))))))
 
- ;;;###autoload
+;;;###autoload
+(defun image-contents-sha1 (filename)
+  "Compute the SHA-1 of the first 4KiB of FILENAME's contents.  The
+first 4KiB does not represent the whole file but was chosen
+because it is fast enough for interactive usage."
+  (with-temp-buffer
+    (insert-file-contents-literally filename nil 0 4096)
+    (sha1 (current-buffer))))
+
+;;;###autoload
 (defun image-supported-file-p (file)
   "Say whether Emacs has native support for displaying TYPE.
 The value is a symbol specifying the image type, or nil if type
diff --git a/lisp/image/image-dired-util.el b/lisp/image/image-dired-util.el
index e17cc6c919f..e5f526fec57 100644
--- a/lisp/image/image-dired-util.el
+++ b/lisp/image/image-dired-util.el
@@ -59,12 +59,6 @@ image-dired-dir
       (message "Thumbnail directory created: %s" image-dired-dir))
     image-dired-dir))
 
-(defun image-dired-contents-sha1 (filename)
-  "Compute the SHA-1 of the first 4KiB of FILENAME's contents."
-  (with-temp-buffer
-    (insert-file-contents-literally filename nil 0 4096)
-    (sha1 (current-buffer))))
-
 (defun image-dired-thumb-name (file)
   "Return absolute file name for thumbnail FILE.
 Depending on the value of `image-dired-thumbnail-storage' and
@@ -99,7 +93,7 @@ image-dired-thumb-name
            (concat (md5 (concat "file://" file)) ".png")
            (expand-file-name thumbdir (xdg-cache-home))))
       (let ((name (if (eq 'sha1-contents image-dired-thumb-naming)
-                      (image-dired-contents-sha1 file)
+                      (image-contents-sha1 file)
                     ;; Defaults to SHA-1 of file name
                     (sha1 file))))
         (cond ((or (eq 'image-dired image-dired-thumbnail-storage)
-- 
2.43.0


[-- Attachment #3: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: 68006@debbugs.gnu.org
> Date: Fri, 29 Dec 2023 12:11:44 +0100
> 
> What about this new patch?  It works for me as intended but might need
> better names and better docs.  Thanks.
> 
> +(defcustom image-mode-eager-cache-flush t
> +  "Non-nil means flush image from cache eagerly.
> +When set to nil, be aware that the image cache could grow fast
> +but an image previously displayed will show faster."

This doc string should explain the effects better, in terms of
user-facing behavior, not in terms of technical aspects of the code.

> -    ;; Discard any stale image data before looking it up again.
> -    (image-flush image)
> +    (if image-mode-eager-cache-flush
> +        ;; Discard any stale image data before looking it up again.
> +        (image-flush image)
> +      ;; Add a content based hash into image spec to be sure that the
> +      ;; cache is updated should the on disk image change.
> +      (when (and filename (file-exists-p filename))
> +        (setq image (append image (list :hash (image-contents-sha1 filename))))))
> +

I'm probably missing something: how would this assure that if the
image file is replaced, we re-read it from disk?

> +(defun image-contents-sha1 (filename)
> +  "Compute the SHA-1 of the first 4KiB of FILENAME's contents.  The
> +first 4KiB does not represent the whole file but was chosen
> +because it is fast enough for interactive usage."

The first line of a doc string should be a single complete sentence.

Thanks.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> -    ;; Discard any stale image data before looking it up again.
>> -    (image-flush image)
>> +    (if image-mode-eager-cache-flush
>> +        ;; Discard any stale image data before looking it up again.
>> +        (image-flush image)
>> +      ;; Add a content based hash into image spec to be sure that the
>> +      ;; cache is updated should the on disk image change.
>> +      (when (and filename (file-exists-p filename))
>> +        (setq image (append image (list :hash (image-contents-sha1 filename))))))
>> +
>
> I'm probably missing something: how would this assure that if the
> image file is replaced, we re-read it from disk?

The logic goes like this:

    - I compute a new hash of the content on disk
    - This hash is add to the spec
    - Now a key in the image cache is computed for this spec
      (i.e. filename, size and *hash* included)
    - If the hash was changed the cache will miss and read the new image
      content.  If hash and size had not changed the cache will hit and
      return the correct image.

Is it more clear?  Or maybe I do not answer your question?

I'll try to address your other remarks in a new version of this patch.
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: 68006@debbugs.gnu.org
>> Date: Fri, 29 Dec 2023 12:11:44 +0100
>> 
>> What about this new patch?  It works for me as intended but might need
>> better names and better docs.  Thanks.
>> 
>> +(defcustom image-mode-eager-cache-flush t
>> +  "Non-nil means flush image from cache eagerly.
>> +When set to nil, be aware that the image cache could grow fast
>> +but an image previously displayed will show faster."
>
> This doc string should explain the effects better, in terms of
> user-facing behavior, not in terms of technical aspects of the code.

And here is a new version of the patch.  Maybe, we need a NEWS entry for
this too.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-some-user-control-in-image-mode-caching.patch --]
[-- Type: text/x-patch, Size: 4255 bytes --]

From 49f1af6333b90a700c52fd4845583c0506d14795 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 29 Dec 2023 11:48:21 +0100
Subject: [PATCH] Add some user control in image-mode caching

Bug#68006

* lisp/image-mode.el (image-mode-eager-cache-flush): New custom.
(image-toggle-display-image): Add a contents hash in image spec
when flushing cache lazily.

* lisp/image/image-dired-util.el (image-dired-thumb-name):
* lisp/image.el (image-contents-sha1): Move
'image-dired-contents-sha1' to 'image-contents-sha1' in toplevel
image library.
---
 lisp/image-mode.el             | 20 ++++++++++++++++++--
 lisp/image.el                  | 11 ++++++++++-
 lisp/image/image-dired-util.el |  8 +-------
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/lisp/image-mode.el b/lisp/image-mode.el
index d5ca6348c92..68be3faaae7 100644
--- a/lisp/image-mode.el
+++ b/lisp/image-mode.el
@@ -93,6 +93,16 @@ image-auto-resize-on-window-resize
   :version "27.1"
   :group 'image)
 
+(defcustom image-mode-eager-cache-flush t
+  "Non-nil means an image is flushed from the cache before being
+read again.  When set to nil, an image spec is not removed from
+the cache so it will be displayed faster the second time.
+
+See `image-cache-eviction-delay'."
+  :type 'boolean
+  :version "30.1"
+  :group 'image)
+
 (defvar-local image-transform-resize nil
   "The image resize operation.
 Non-nil to resize the image upon first display.
@@ -954,8 +964,14 @@ image-toggle-display-image
                   (plist-put (cdr image) :width
                              (plist-get (cdr image) :max-width)))))
 
-    ;; Discard any stale image data before looking it up again.
-    (image-flush image)
+    (if image-mode-eager-cache-flush
+        ;; Discard any stale image data before looking it up again.
+        (image-flush image)
+      ;; Add a content based hash into image spec to be sure that the
+      ;; cache is updated should the on disk image change.
+      (when (and filename (file-exists-p filename))
+        (setq image (append image (list :hash (image-contents-sha1 filename))))))
+
     (setq image (append image (image-transform-properties image)))
     (setq props
 	  `(display ,image
diff --git a/lisp/image.el b/lisp/image.el
index e20fbcf4c98..0f1c74d9250 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -401,7 +401,16 @@ image-type-from-file-name
 	    ;; If nothing seems to be supported, return first type that matched.
 	    (or first (setq first type))))))))
 
- ;;;###autoload
+;;;###autoload
+(defun image-contents-sha1 (filename)
+  "Compute the SHA-1 of the first 4KiB of FILENAME's contents.
+The first 4KiB does not represent the whole file but was chosen
+because it is fast enough for interactive usage."
+  (with-temp-buffer
+    (insert-file-contents-literally filename nil 0 4096)
+    (sha1 (current-buffer))))
+
+;;;###autoload
 (defun image-supported-file-p (file)
   "Say whether Emacs has native support for displaying TYPE.
 The value is a symbol specifying the image type, or nil if type
diff --git a/lisp/image/image-dired-util.el b/lisp/image/image-dired-util.el
index e17cc6c919f..e5f526fec57 100644
--- a/lisp/image/image-dired-util.el
+++ b/lisp/image/image-dired-util.el
@@ -59,12 +59,6 @@ image-dired-dir
       (message "Thumbnail directory created: %s" image-dired-dir))
     image-dired-dir))
 
-(defun image-dired-contents-sha1 (filename)
-  "Compute the SHA-1 of the first 4KiB of FILENAME's contents."
-  (with-temp-buffer
-    (insert-file-contents-literally filename nil 0 4096)
-    (sha1 (current-buffer))))
-
 (defun image-dired-thumb-name (file)
   "Return absolute file name for thumbnail FILE.
 Depending on the value of `image-dired-thumbnail-storage' and
@@ -99,7 +93,7 @@ image-dired-thumb-name
            (concat (md5 (concat "file://" file)) ".png")
            (expand-file-name thumbdir (xdg-cache-home))))
       (let ((name (if (eq 'sha1-contents image-dired-thumb-naming)
-                      (image-dired-contents-sha1 file)
+                      (image-contents-sha1 file)
                     ;; Defaults to SHA-1 of file name
                     (sha1 file))))
         (cond ((or (eq 'image-dired image-dired-thumbnail-storage)
-- 
2.43.0


[-- Attachment #3: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Stefan Kangas]

Manuel Giraud via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> And here is a new version of the patch.  Maybe, we need a NEWS entry for
> this too.

If we have an option, then yes we should add it to NEWS too.

But I'm not sure about this one:

Flushing the image cache all the time is fine for images that are 4KB in
size, but these days we routinely work with images that are several
megabytes in size (to give an idea, searching online tells me that
Twitter allows uploading photos up to 2 MB, Facebook up to 15 MB).
Furthermore, my experience with image-dired has taught me that Emacs is
terribly slow when compared to all other programs capable of viewing
images out there.  So I think we should think a bit more about this one.

Taking a step back, why are images treated differently from other
buffers?  If the risk is that the image changes on disk without us
noticing, then users should need to either run `revert-buffer' or enable
`auto-revert-mode'.  If we are talking about images that are inline in a
buffer, the cache should be flushed only when the buffer itself is
reverted.  What am I missing?

Lastly, I'm not sure about the "hash the first 4 KB of an image"
heuristic, for starters because images can be several megabytes in size.
Would it not be both more accurate and faster to check the mtime and/or
the size of the file?  Or do we need different heuristic for different
image formats?  (Maybe JPEG changes the first 4 KB on save, but I don't
think all bitmap formats do.)  But wouldn't it be even better to use the
notify system when possible, like with `auto-revert-use-notify'?

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 30 Dec 2023 15:57:28 -0800
> Cc: 68006@debbugs.gnu.org
> 
> Taking a step back, why are images treated differently from other
> buffers?  If the risk is that the image changes on disk without us
> noticing, then users should need to either run `revert-buffer' or enable
> `auto-revert-mode'.  If we are talking about images that are inline in a
> buffer, the cache should be flushed only when the buffer itself is
> reverted.  What am I missing?

See my explanation of the purpose of this particular cache here:

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=68006#14

In a nutshell, this cache is ephemeral anyway, and will be flushed at
some arbitrary time whether we want it or not, because its purpose is
not what you think it is.

In any case, if you intend to not flush the cache at all, then what
does that mean for Emacs sessions running for days and weeks, let
alone months, on end? will they keep these images in memory forever?
Or should GC sometimes evict those images from the cache, and if so,
under what conditions?

As for the differences between images and other buffers, there are
some:

  . many buffers are smaller than images nowadays
  . buffers with images are in many cases showing text we are
    interested in temporarily, like Web pages (program source code
    rarely if ever includes images, right?)

bug#68006: 30.0.50; Image-mode speed

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefankangas@gmail.com>
>> Date: Sat, 30 Dec 2023 15:57:28 -0800
>> Cc: 68006@debbugs.gnu.org
>>
>> Taking a step back, why are images treated differently from other
>> buffers?  If the risk is that the image changes on disk without us
>> noticing, then users should need to either run `revert-buffer' or enable
>> `auto-revert-mode'.  If we are talking about images that are inline in a
>> buffer, the cache should be flushed only when the buffer itself is
>> reverted.  What am I missing?
>
> See my explanation of the purpose of this particular cache here:
>
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=68006#14

I read that, yes.  Quoting what you wrote there, and I hope I'm quoting
the most relevant part:

    > The real purpose of the image cache is not to hold the image in
    > memory for displaying it again and again, the purpose is to allow
    > the display engine to generate the pixels once, then reuse those
    > pixels during the current redisplay cycle or a few following
    > redisplay cycles.

Basically, I don't see how this difference relates to manually evicting
the cache or not.

> In a nutshell, this cache is ephemeral anyway, and will be flushed at
> some arbitrary time whether we want it or not, because its purpose is
> not what you think it is.

If it is flushed anyways, then that is exactly what we want here, I
think.  The idea is to flush it less often, AFAIU.

> In any case, if you intend to not flush the cache at all, then what
> does that mean for Emacs sessions running for days and weeks, let
> alone months, on end? will they keep these images in memory forever?
> Or should GC sometimes evict those images from the cache, and if so,
> under what conditions?

Are you saying that, if this particular call is removed, we will never
flush the cache (i.e. we will have memory leaks)?

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefankangas@gmail.com> writes:

[...]

>> In a nutshell, this cache is ephemeral anyway, and will be flushed at
>> some arbitrary time whether we want it or not, because its purpose is
>> not what you think it is.
>
> If it is flushed anyways, then that is exactly what we want here, I
> think.  The idea is to flush it less often, AFAIU.

Yes that's it: flush less often (at the expense of more used memory of
course)

>> In any case, if you intend to not flush the cache at all, then what
>> does that mean for Emacs sessions running for days and weeks, let
>> alone months, on end? will they keep these images in memory forever?
>> Or should GC sometimes evict those images from the cache, and if so,
>> under what conditions?
>
> Are you saying that, if this particular call is removed, we will never
> flush the cache (i.e. we will have memory leaks)?

I don't know what Eli had in mind but removing this particular call
would not remove the timely cache eviction mechanism (via
'image-cache-eviction-delay')
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Mon, 1 Jan 2024 16:19:31 -0800
> Cc: manuel@ledu-giraud.fr, 68006@debbugs.gnu.org
> 
>     > The real purpose of the image cache is not to hold the image in
>     > memory for displaying it again and again, the purpose is to allow
>     > the display engine to generate the pixels once, then reuse those
>     > pixels during the current redisplay cycle or a few following
>     > redisplay cycles.
> 
> Basically, I don't see how this difference relates to manually evicting
> the cache or not.

My point was that you are expecting from this cache something that it
wasn't intended to provide.  As a corollary, I think if we want to
speed up paging through images, we should design and implement a
separate cache, which is designed to support the use case of going
through many images one by one, back and forth.

> > In a nutshell, this cache is ephemeral anyway, and will be flushed at
> > some arbitrary time whether we want it or not, because its purpose is
> > not what you think it is.
> 
> If it is flushed anyways, then that is exactly what we want here, I
> think.  The idea is to flush it less often, AFAIU.

My point is that you cannot currently control when it is flushed, not
fully anyway.  Again, the reason is that the cache was not for the
purpose we need caching images in the case that's being discussed
here.

> > In any case, if you intend to not flush the cache at all, then what
> > does that mean for Emacs sessions running for days and weeks, let
> > alone months, on end? will they keep these images in memory forever?
> > Or should GC sometimes evict those images from the cache, and if so,
> > under what conditions?
> 
> Are you saying that, if this particular call is removed, we will never
> flush the cache

Not just this call: you'd need to remove or disable the code which
evicts images from the cache "when the display engine feels like it".
And if you do that, then yes, the images will never be flushed, since
there will be nothing the flushes the cache.

> (i.e. we will have memory leaks)?

Not a memory leak, but memory that is never released.

Basically, we lack a good way of knowing that a given cached image is
no longer needed.  The cache as it exists now doesn't care, since it
isn't supposed to hold the image long enough for that to matter; but
if you want a tighter control on when an image is uncached, we need to
design and implement a way for that, and the main stumbling block in
that case is how to define conditions under which an image can be
evicted from the cache.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> My point was that you are expecting from this cache something that it
> wasn't intended to provide.  As a corollary, I think if we want to
> speed up paging through images, we should design and implement a
> separate cache, which is designed to support the use case of going
> through many images one by one, back and forth.

Do you think we need a completely different cache or just some handles
on the current one?
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Stefan Kangas <stefankangas@gmail.com>,  68006@debbugs.gnu.org
> Date: Tue, 02 Jan 2024 17:04:09 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > My point was that you are expecting from this cache something that it
> > wasn't intended to provide.  As a corollary, I think if we want to
> > speed up paging through images, we should design and implement a
> > separate cache, which is designed to support the use case of going
> > through many images one by one, back and forth.
> 
> Do you think we need a completely different cache or just some handles
> on the current one?

A different cache, I think.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: Stefan Kangas <stefankangas@gmail.com>,  68006@debbugs.gnu.org
>> Date: Tue, 02 Jan 2024 17:04:09 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > My point was that you are expecting from this cache something that it
>> > wasn't intended to provide.  As a corollary, I think if we want to
>> > speed up paging through images, we should design and implement a
>> > separate cache, which is designed to support the use case of going
>> > through many images one by one, back and forth.
>> 
>> Do you think we need a completely different cache or just some handles
>> on the current one?
>
> A different cache, I think.

Hi,

I've tried to think about it a bit but I don't understand why do you
think it should be a different one.  With `image-cache-eviction-delay'
being 300 seconds by default, I think this is enough time for this usage
too.
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
> Date: Thu, 04 Jan 2024 17:47:42 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Do you think we need a completely different cache or just some handles
> >> on the current one?
> >
> > A different cache, I think.
> 
> Hi,
> 
> I've tried to think about it a bit but I don't understand why do you
> think it should be a different one.

Because its purpose is different, and the conditions to flush are also
different.

> With `image-cache-eviction-delay' being 300 seconds by default, I
> think this is enough time for this usage too.

I'm not sure I understand why time since caching is relevant for the
purpose that you want to cache images.  Suppose there's an image that
should be displayed during the entire session -- how does it make
sense to remove it from the cache after 5 minutes?  OTOH, another
image that is displayed just once doesn't need to be cached for more
than a few seconds.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> I've tried to think about it a bit but I don't understand why do you
>> think it should be a different one.
>
> Because its purpose is different, and the conditions to flush are also
> different.
>
>> With `image-cache-eviction-delay' being 300 seconds by default, I
>> think this is enough time for this usage too.
>
> I'm not sure I understand why time since caching is relevant for the
> purpose that you want to cache images.  Suppose there's an image that
> should be displayed during the entire session -- how does it make
> sense to remove it from the cache after 5 minutes?  OTOH, another
> image that is displayed just once doesn't need to be cached for more
> than a few seconds.

My idea was just to have image-mode to feel that it display images
faster.  In the first time, by caching already displayed images and
maybe, in a second time, by doing some prefetching of nearby images.
For this usage, I thought that 5 minutes was also a quite good value.

But anyway, you're thinking about another image cache completely user
manageable?  I guess it is a much more harder problem to tackle.  For
starters, as the current image-mode is using the 'image' display
property which uses the current image cache, I imagine that it should
then be replaced by something else?
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
> Date: Thu, 04 Jan 2024 19:42:53 +0100
> 
> But anyway, you're thinking about another image cache completely user
> manageable?

Yes.

> I guess it is a much more harder problem to tackle.

Probably.  But anything else will be IMO a kludge.  (We do have
kludges in Emacs, so we could have one here as well, of course.  I
just thought that it would be good to avoid it if possible).

> For starters, as the current image-mode is using the 'image' display
> property which uses the current image cache, I imagine that it
> should then be replaced by something else?

Yes, probably.  And if you are going for the simple solution, please
keep in mind that image-cache-eviction-delay is a global variable, and
the images are cached on a per-frame basis.  So if image-mode enlarges
image-cache-eviction-delay, it will affect all the images, not just
those in image-mode buffers.  One more reason why I think we should
provide a separate and differently managed cache for this purpose.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
>> Date: Thu, 04 Jan 2024 19:42:53 +0100
>> 
>> But anyway, you're thinking about another image cache completely user
>> manageable?
>
> Yes.
>
>> I guess it is a much more harder problem to tackle.
>
> Probably.  But anything else will be IMO a kludge.  (We do have
> kludges in Emacs, so we could have one here as well, of course.  I
> just thought that it would be good to avoid it if possible).

Ok.

>> For starters, as the current image-mode is using the 'image' display
>> property which uses the current image cache, I imagine that it
>> should then be replaced by something else?

Thinking about that, maybe we could have this other cache on top of the
current one (and still on top of the image display property).

> Yes, probably.  And if you are going for the simple solution, please
> keep in mind that image-cache-eviction-delay is a global variable, and
> the images are cached on a per-frame basis.  So if image-mode enlarges
> image-cache-eviction-delay, it will affect all the images, not just
> those in image-mode buffers.  One more reason why I think we should
> provide a separate and differently managed cache for this purpose.

Ok.  Maybe we should re-use the current C cache code with a "nice"
interface for this too.
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
> Date: Thu, 04 Jan 2024 20:16:19 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Yes, probably.  And if you are going for the simple solution, please
> > keep in mind that image-cache-eviction-delay is a global variable, and
> > the images are cached on a per-frame basis.  So if image-mode enlarges
> > image-cache-eviction-delay, it will affect all the images, not just
> > those in image-mode buffers.  One more reason why I think we should
> > provide a separate and differently managed cache for this purpose.
> 
> Ok.  Maybe we should re-use the current C cache code with a "nice"
> interface for this too.

Yes, that can be done.  But we might need to store additional
information there, like the buffer of the image, for example.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
>> Date: Thu, 04 Jan 2024 20:16:19 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Yes, probably.  And if you are going for the simple solution, please
>> > keep in mind that image-cache-eviction-delay is a global variable, and
>> > the images are cached on a per-frame basis.  So if image-mode enlarges
>> > image-cache-eviction-delay, it will affect all the images, not just
>> > those in image-mode buffers.  One more reason why I think we should
>> > provide a separate and differently managed cache for this purpose.
>> 
>> Ok.  Maybe we should re-use the current C cache code with a "nice"
>> interface for this too.
>
> Yes, that can be done.

Ok.  More thinking: my idea is to have an interface to create/destroy
image caches and feed/query them.  And why not have a Lisp interface to
it so something like image-mode could use this to manage its own cache.

> But we might need to store additional information there, like the
> buffer of the image, for example.

I don't understand.  Where do you think that the buffer of the image
should be stored?
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
> Date: Fri, 05 Jan 2024 11:50:24 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Ok.  Maybe we should re-use the current C cache code with a "nice"
> >> interface for this too.
> >
> > Yes, that can be done.
> 
> Ok.  More thinking: my idea is to have an interface to create/destroy
> image caches and feed/query them.  And why not have a Lisp interface to
> it so something like image-mode could use this to manage its own cache.

Yes, of course.  We have a Lisp API for the existing cache as well,
but this new one will need it much more, since we basically rely on
the Lisp programs to manage this new cache.

> > But we might need to store additional information there, like the
> > buffer of the image, for example.
> 
> I don't understand.  Where do you think that the buffer of the image
> should be stored?

In the cache, together with the image, I think.  How else would the
Lisp programs know which images "belong" to them, and when to uncache
them?  I guess some other information relevant to the cache-removal
decision will also need to be stored there.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> > But we might need to store additional information there, like the
>> > buffer of the image, for example.
>> 
>> I don't understand.  Where do you think that the buffer of the image
>> should be stored?
>
> In the cache, together with the image, I think.

You mean in struct image_cache?

> How else would the Lisp programs know which images "belong" to them,
> and when to uncache them?

I don't know exactly.  My idea was that an image-mode buffer has (buffer
local I guess) variable that will hold its image cache.

> I guess some other information relevant to the cache-removal decision
> will also need to be stored there.

Yes, I imagine.
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
> Date: Fri, 05 Jan 2024 14:26:20 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > But we might need to store additional information there, like the
> >> > buffer of the image, for example.
> >> 
> >> I don't understand.  Where do you think that the buffer of the image
> >> should be stored?
> >
> > In the cache, together with the image, I think.
> 
> You mean in struct image_cache?

Yes.

> > How else would the Lisp programs know which images "belong" to them,
> > and when to uncache them?
> 
> I don't know exactly.  My idea was that an image-mode buffer has (buffer
> local I guess) variable that will hold its image cache.

That is also possible, but then will the cache itself be implemented
only in Lisp?  I assumed at least some of it will be in C.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
>> Date: Fri, 05 Jan 2024 14:26:20 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> > But we might need to store additional information there, like the
>> >> > buffer of the image, for example.
>> >> 
>> >> I don't understand.  Where do you think that the buffer of the image
>> >> should be stored?
>> >
>> > In the cache, together with the image, I think.
>> 
>> You mean in struct image_cache?
>
> Yes.
>
>> > How else would the Lisp programs know which images "belong" to them,
>> > and when to uncache them?
>> 
>> I don't know exactly.  My idea was that an image-mode buffer has (buffer
>> local I guess) variable that will hold its image cache.
>
> That is also possible, but then will the cache itself be implemented
> only in Lisp?  I assumed at least some of it will be in C.

I also imagine it would be mostly in C.  But looking at the code, I
think I have overlooked something: lookup_image is what currently does
most of the work (with caching) and this is in called in many places in
"xdisp.c".  Of course, I think we should keep that... but then how could
we design this new cache?
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
> Date: Fri, 05 Jan 2024 15:35:42 +0100
> 
> I also imagine it would be mostly in C.  But looking at the code, I
> think I have overlooked something: lookup_image is what currently does
> most of the work (with caching) and this is in called in many places in
> "xdisp.c".  Of course, I think we should keep that... but then how could
> we design this new cache?

For example, lookup_image could look in both the existing and this new
cache.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
>> Date: Fri, 05 Jan 2024 15:35:42 +0100
>> 
>> I also imagine it would be mostly in C.  But looking at the code, I
>> think I have overlooked something: lookup_image is what currently does
>> most of the work (with caching) and this is in called in many places in
>> "xdisp.c".  Of course, I think we should keep that... but then how could
>> we design this new cache?
>
> For example, lookup_image could look in both the existing and this new
> cache.

Yes why not.  So your idea is to have just one more other cache that we
can access differently than the current one.  I was thinking more of
something to let Lisp programs create their own image cache but that may
be less practical.
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

Here is a diff of where I'm headed to.  'user-image-cache' is not used
at all yet.  I imagine that a user could fill it with a call to
'make-image-cache' (when it exists).  Anyway, WDYT?

diff --git a/src/image.c b/src/image.c
index 252b83da992..27d564fb49c 100644
--- a/src/image.c
+++ b/src/image.c
@@ -2086,7 +2086,7 @@ image_alloc_image_color (struct frame *f, struct image *img,
 			     Image Cache
  ***********************************************************************/
 
-static void cache_image (struct frame *f, struct image *img);
+static void cache_image (struct image_cache **pc, struct image *img);
 
 /* Return a new, initialized image cache that is allocated from the
    heap.  Call free_image_cache to free an image cache.  */
@@ -2103,15 +2103,14 @@ make_image_cache (void)
   return c;
 }
 
-/* Find an image matching SPEC in the cache, and return it.  If no
+/* Find an image matching SPEC in the cache C, and return it.  If no
    image is found, return NULL.  */
 static struct image *
-search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
+search_image_cache (struct image_cache *c, Lisp_Object spec, EMACS_UINT hash,
                     unsigned long foreground, unsigned long background,
                     int font_size, char *font_family, bool ignore_colors)
 {
   struct image *img;
-  struct image_cache *c = FRAME_IMAGE_CACHE (f);
   int i = hash % IMAGE_CACHE_BUCKETS_SIZE;
 
   if (!c) return NULL;
@@ -2185,12 +2184,13 @@ uncache_image (struct frame *f, Lisp_Object spec)
 {
   struct image *img;
   EMACS_UINT hash = sxhash (filter_image_spec (spec));
+  struct image_cache *cache = FRAME_IMAGE_CACHE (f);
 
   /* Because the background colors are based on the current face, we
      can have multiple copies of an image with the same spec. We want
      to remove them all to ensure the user doesn't see an old version
      of the image when the face changes.  */
-  while ((img = search_image_cache (f, spec, hash, 0, 0, 0, NULL, true)))
+  while ((img = search_image_cache (cache, spec, hash, 0, 0, 0, NULL, true)))
     {
       free_image (f, img);
       /* As display glyphs may still be referring to the image ID, we
@@ -3359,6 +3359,7 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
   unsigned long background = face->background;
   int font_size = face->font->pixel_size;
   char *font_family = SSDATA (face->lface[LFACE_FAMILY_INDEX]);
+  struct image_cache *cache;
 
   /* F must be a window-system frame, and SPEC must be a valid image
      specification.  */
@@ -3367,7 +3368,13 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
 
   /* Look up SPEC in the hash table of the image cache.  */
   hash = sxhash (filter_image_spec (spec));
-  img = search_image_cache (f, spec, hash, foreground, background,
+
+  if (!NILP (Vuser_image_cache))
+    cache = XUNTAG (Vuser_image_cache, Lisp_Vectorlike, struct image_cache);
+  else
+    cache = FRAME_IMAGE_CACHE (f);
+  
+  img = search_image_cache (cache, spec, hash, foreground, background,
 			    font_size, font_family, false);
   if (img && img->load_failed_p)
     {
@@ -3380,7 +3387,7 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
     {
       block_input ();
       img = make_image (spec, hash);
-      cache_image (f, img);
+      cache_image (&cache, img);
       img->face_foreground = foreground;
       img->face_background = background;
       img->face_font_size = font_size;
@@ -3470,16 +3477,17 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
 }
 
 
-/* Cache image IMG in the image cache of frame F.  */
+/* Cache image IMG in the image cache C.  */
 
 static void
-cache_image (struct frame *f, struct image *img)
+cache_image (struct image_cache **pc, struct image *img)
 {
-  struct image_cache *c = FRAME_IMAGE_CACHE (f);
+  struct image_cache *c;
   ptrdiff_t i;
 
-  if (!c)
-    c = FRAME_IMAGE_CACHE (f) = make_image_cache ();
+  if (!*pc)
+    *pc = make_image_cache ();
+  c = *pc;
 
   /* Find a free slot in c->images.  */
   for (i = 0; i < c->used; ++i)
@@ -12975,6 +12983,10 @@ syms_of_image (void)
 
 The function `clear-image-cache' disregards this variable.  */);
   Vimage_cache_eviction_delay = make_fixnum (300);
+
+  DEFVAR_LISP ("user-image-cache", Vuser_image_cache,
+    doc: /* TBD.  */);
+  Vuser_image_cache = Qnil;
 #ifdef HAVE_IMAGEMAGICK
   DEFVAR_INT ("imagemagick-render-type", imagemagick_render_type,
     doc: /* Integer indicating which ImageMagick rendering method to use.


-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi,

I'm trying to revisit this idea of having a user controlled image cache.
The goal is to have, for instance, image-mode use it because (hopefully)
it would have a better idea of when to cache/uncache images for its
usage.

Here is a patch of such a prototype « user image cache ».

For the moment, I wanted to keep it minimal so the only interface to use
it goes as follow.  When a user (or a program) use the image attribute
":ttl TIME_IN_SECONDS" with `create-image' such image will be stored in
the « user cache » and not in the internal one.  Images lookups use both
caches (internal and internal, in that order).  A negative
TIME_IN_SECONDS means « cache this image forever ».

I think there will be a need for user function that completely flushes
the images in this new cache.  But do you think we need others commands
for such interface?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-User-controlled-image-cache.patch --]
[-- Type: text/x-patch, Size: 14043 bytes --]

From a51924c295e2141ef7b712e98d9c45adcae3bf96 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Thu, 17 Oct 2024 11:27:56 +0200
Subject: [PATCH] User controlled image cache

---
 src/dispextern.h |   4 +
 src/frame.h      |   4 +
 src/image.c      | 205 +++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 188 insertions(+), 25 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index cc248a4472e..69e4d1171b3 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3141,6 +3141,9 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
      in prepare_image_for_display.  */
   struct timespec timestamp;
 
+  /* The end of life for an image in the user's image cache */
+  struct timespec eol;
+
   /* Pixmaps of the image.  */
   Emacs_Pixmap pixmap, mask;
 
@@ -3632,6 +3635,7 @@ #define TRY_WINDOW_IGNORE_FONTS_CHANGE	(1 << 1)
 #ifdef HAVE_WINDOW_SYSTEM
 
 extern void clear_image_cache (struct frame *, Lisp_Object);
+extern void clear_user_image_cache (struct frame *);
 extern ptrdiff_t image_bitmap_pixmap (struct frame *, ptrdiff_t);
 extern void image_reference_bitmap (struct frame *, ptrdiff_t);
 extern ptrdiff_t image_create_bitmap_from_data (struct frame *, char *,
diff --git a/src/frame.h b/src/frame.h
index 1d920d1a6bc..984568234f1 100644
--- a/src/frame.h
+++ b/src/frame.h
@@ -292,6 +292,9 @@ #define EMACS_FRAME_H
   /* Cache of realized images, which may be shared with other
      frames.  */
   struct image_cache *image_cache;
+
+  /* Another image cache that can be managed from Emacs. */
+  struct image_cache *user_image_cache;
 #endif /* HAVE_WINDOW_SYSTEM */
 
   /* Tab-bar item index of the item on which a mouse button was pressed.  */
@@ -916,6 +919,7 @@ #define FRAME_KBOARD(f) ((f)->terminal->kboard)
 
 /* Return a pointer to the image cache of frame F.  */
 #define FRAME_IMAGE_CACHE(F) ((F)->image_cache)
+#define FRAME_USER_IMAGE_CACHE(F) ((F)->user_image_cache)
 
 #define XFRAME(p) \
   (eassert (FRAMEP (p)), XUNTAG (p, Lisp_Vectorlike, struct frame))
diff --git a/src/image.c b/src/image.c
index 34936977a40..8676ba27632 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1744,6 +1744,7 @@ make_image (Lisp_Object spec, EMACS_UINT hash)
 {
   struct image *img = xzalloc (sizeof *img);
   Lisp_Object file = image_spec_value (spec, QCfile, NULL);
+  Lisp_Object ttl = image_spec_value (spec, QCttl, NULL);
 
   eassert (valid_image_p (spec));
   img->dependencies = NILP (file) ? Qnil : list1 (file);
@@ -1754,19 +1755,35 @@ make_image (Lisp_Object spec, EMACS_UINT hash)
   img->ascent = DEFAULT_IMAGE_ASCENT;
   img->hash = hash;
   img->corners[BOT_CORNER] = -1;  /* Full image */
+  if (! NILP (ttl) && FIXNUMP (ttl))
+    {
+      struct timespec now = current_timespec ();
+      struct timespec ttl_spec;
+
+      /* A negative TTL is used to mean forever.  */
+      if (XFIXNUM (ttl) < 0)
+	(img->eol).tv_sec = -1;
+      else
+	{
+	  ttl_spec.tv_sec = XFIXNUM (ttl);
+	  ttl_spec.tv_nsec = 0;
+	  img->eol = timespec_add(now, ttl_spec);
+	}
+    }
+  else
+    (img->eol).tv_sec = (img->eol).tv_nsec = 0;
+
   return img;
 }
 
 
-/* Free image IMG which was used on frame F, including its resources.  */
+/* Free image IMG from the cache C, including its resources on frame F.  */
 
 static void
-free_image (struct frame *f, struct image *img)
+free_image (struct image_cache *c, struct frame *f, struct image *img)
 {
   if (img)
     {
-      struct image_cache *c = FRAME_IMAGE_CACHE (f);
-
       /* Remove IMG from the hash table of its cache.  */
       if (img->prev)
 	img->prev->next = img->next;
@@ -2179,6 +2196,7 @@ image_alloc_image_color (struct frame *f, struct image *img,
  ***********************************************************************/
 
 static void cache_image (struct frame *f, struct image *img);
+static void user_cache_image (struct frame *f, struct image *img);
 
 /* Return a new, initialized image cache that is allocated from the
    heap.  Call free_image_cache to free an image cache.  */
@@ -2197,15 +2215,14 @@ make_image_cache (void)
   return c;
 }
 
-/* Find an image matching SPEC in the cache, and return it.  If no
+/* Find an image matching SPEC in the cache C, and return it.  If no
    image is found, return NULL.  */
 static struct image *
-search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
+search_image_cache (struct image_cache *c, Lisp_Object spec, EMACS_UINT hash,
                     unsigned long foreground, unsigned long background,
                     int font_size, char *font_family, bool ignore_colors)
 {
   struct image *img;
-  struct image_cache *c = FRAME_IMAGE_CACHE (f);
   int i = hash % IMAGE_CACHE_BUCKETS_SIZE;
 
   if (!c) return NULL;
@@ -2258,11 +2275,13 @@ filter_image_spec (Lisp_Object spec)
 	  spec = XCDR (spec);
 
 	  /* Some animation-related data doesn't affect display, but
-	     breaks the image cache.  Filter those out.  */
+	     breaks the image cache.  Filter those out.  Also filter out
+	     the user's cache time to live.  */
 	  if (!(EQ (key, QCanimate_buffer)
 		|| EQ (key, QCanimate_tardiness)
 		|| EQ (key, QCanimate_position)
-		|| EQ (key, QCanimate_multi_frame_data)))
+		|| EQ (key, QCanimate_multi_frame_data)
+		|| EQ (key, QCttl)))
 	    {
 	      out = Fcons (value, out);
 	      out = Fcons (key, out);
@@ -2279,14 +2298,15 @@ uncache_image (struct frame *f, Lisp_Object spec)
 {
   struct image *img;
   EMACS_UINT hash = sxhash (filter_image_spec (spec));
+  struct image_cache *c = FRAME_IMAGE_CACHE (f);
 
   /* Because the background colors are based on the current face, we
      can have multiple copies of an image with the same spec. We want
      to remove them all to ensure the user doesn't see an old version
      of the image when the face changes.  */
-  while ((img = search_image_cache (f, spec, hash, 0, 0, 0, NULL, true)))
+  while ((img = search_image_cache (c, spec, hash, 0, 0, 0, NULL, true)))
     {
-      free_image (f, img);
+      free_image (c, f, img);
       /* As display glyphs may still be referring to the image ID, we
 	 must garbage the frame (Bug#6426).  */
       SET_FRAME_GARBAGED (f);
@@ -2310,7 +2330,7 @@ free_image_cache (struct frame *f)
   eassert (c->refcount == 0);
 
   for (i = 0; i < c->used; ++i)
-    free_image (f, c->images[i]);
+    free_image (c, f, c->images[i]);
   xfree (c->images);
   xfree (c->buckets);
   xfree (c);
@@ -2346,7 +2366,7 @@ clear_image_cache (struct frame *f, Lisp_Object filter)
 	      if (img && (EQ (Qt, filter)
 			  || !NILP (Fmember (filter, img->dependencies))))
 		{
-		  free_image (f, img);
+		  free_image (c, f, img);
 		  ++nfreed;
 		}
 	    }
@@ -2377,7 +2397,7 @@ clear_image_cache (struct frame *f, Lisp_Object filter)
 	      struct image *img = c->images[i];
 	      if (img && timespec_cmp (img->timestamp, old) < 0)
 		{
-		  free_image (f, img);
+		  free_image (c, f, img);
 		  ++nfreed;
 		}
 	    }
@@ -2405,6 +2425,56 @@ clear_image_cache (struct frame *f, Lisp_Object filter)
     }
 }
 
+void
+clear_user_image_cache (struct frame *f)
+{
+  struct image_cache *c = FRAME_USER_IMAGE_CACHE (f);
+
+  if (c && !f->inhibit_clear_image_cache)
+    {
+      ptrdiff_t i, nfreed = 0;
+      struct timespec t = current_timespec ();
+
+      /* Block input so that we won't be interrupted by a SIGIO
+	 while being in an inconsistent state.  */
+      block_input ();
+
+      /* Filter image cache based on image's time to live.  */
+      for (i = 0; i < c->used; ++i)
+	{
+	  struct image *img = c->images[i];
+	  if (img)
+	    {
+	      if (((img->eol).tv_sec >= 0) && timespec_cmp (t, img->eol) > 0)
+		{
+		  free_image (c, f, img);
+		  ++nfreed;
+		}
+	    }
+	}
+
+      /* We may be clearing the image cache because, for example,
+	 Emacs was iconified for a longer period of time.  In that
+	 case, current matrices may still contain references to
+	 images freed above.  So, clear these matrices.  */
+      if (nfreed)
+	{
+	  Lisp_Object tail, frame;
+
+	  FOR_EACH_FRAME (tail, frame)
+	    {
+	      struct frame *fr = XFRAME (frame);
+	      if (FRAME_USER_IMAGE_CACHE (fr) == c)
+		clear_current_matrices (fr);
+	    }
+
+	  windows_or_buffers_changed = 19;
+	}
+
+      unblock_input ();
+    }
+}
+
 void
 clear_image_caches (Lisp_Object filter)
 {
@@ -2415,7 +2485,10 @@ clear_image_caches (Lisp_Object filter)
   Lisp_Object tail, frame;
   FOR_EACH_FRAME (tail, frame)
     if (FRAME_WINDOW_P (XFRAME (frame)))
-      clear_image_cache (XFRAME (frame), filter);
+      {
+	clear_image_cache (XFRAME (frame), filter);
+	clear_user_image_cache (XFRAME (frame));
+      }
 }
 
 DEFUN ("clear-image-cache", Fclear_image_cache, Sclear_image_cache,
@@ -2444,7 +2517,11 @@ DEFUN ("clear-image-cache", Fclear_image_cache, Sclear_image_cache,
   if (! (NILP (filter) || FRAMEP (filter)))
     clear_image_caches (filter);
   else
-    clear_image_cache (decode_window_system_frame (filter), Qt);
+    {
+      struct frame *decoded_frame = decode_window_system_frame (filter);
+      clear_image_cache (decoded_frame, Qt);
+      clear_user_image_cache (decoded_frame);
+    }
 
   /* Also clear the animation caches.  */
   image_prune_animation_caches (true);
@@ -2501,9 +2578,8 @@ image_size_in_bytes (struct image *img)
 }
 
 static size_t
-image_frame_cache_size (struct frame *f)
+image_frame_cache_size (struct image_cache *c)
 {
-  struct image_cache *c = FRAME_IMAGE_CACHE (f);
   if (!c)
     return 0;
 
@@ -3495,6 +3571,7 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
   unsigned long background = face->background;
   int font_size = face->font->pixel_size;
   char *font_family = SSDATA (face->lface[LFACE_FAMILY_INDEX]);
+  struct image_cache *c;
 
   /* F must be a window-system frame, and SPEC must be a valid image
      specification.  */
@@ -3503,20 +3580,39 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
 
   /* Look up SPEC in the hash table of the image cache.  */
   hash = sxhash (filter_image_spec (spec));
-  img = search_image_cache (f, spec, hash, foreground, background,
+  c = FRAME_IMAGE_CACHE (f);
+  img = search_image_cache (c, spec, hash, foreground, background,
 			    font_size, font_family, false);
   if (img && img->load_failed_p)
     {
-      free_image (f, img);
+      free_image (c, f, img);
       img = NULL;
     }
 
-  /* If not found, create a new image and cache it.  */
+  /* If not found, try the user's image cache.  */
+  if (img == NULL)
+    {
+      c = FRAME_USER_IMAGE_CACHE (f);
+      img = search_image_cache (c, spec, hash, foreground, background,
+				font_size, font_family, false);
+      if (img && img->load_failed_p)
+	{
+	  free_image (c, f, img);
+	  img = NULL;
+	}
+    }
+
+  /* If not found again, create a new image and cache it.  */
   if (img == NULL)
     {
       block_input ();
       img = make_image (spec, hash);
-      cache_image (f, img);
+      /* If image's end of life is set store it in the user's image
+	 cache instead.  */
+      if ((img->eol).tv_sec != 0)
+	user_cache_image (f, img);
+      else
+	cache_image (f, img);
       img->face_foreground = foreground;
       img->face_background = background;
       img->face_font_size = font_size;
@@ -3647,6 +3743,42 @@ cache_image (struct frame *f, struct image *img)
   c->buckets[i] = img;
 }
 
+static void
+user_cache_image (struct frame *f, struct image *img)
+{
+  struct image_cache *c = FRAME_USER_IMAGE_CACHE (f);
+  ptrdiff_t i;
+
+  if (!c)
+    {
+      c = FRAME_USER_IMAGE_CACHE (f) = make_image_cache ();
+      c->refcount++;
+    }
+
+  /* Find a free slot in c->images.  */
+  for (i = 0; i < c->used; ++i)
+    if (c->images[i] == NULL)
+      break;
+
+  /* If no free slot found, maybe enlarge c->images.  */
+  if (i == c->used && c->used == c->size)
+    c->images = xpalloc (c->images, &c->size, 1, -1, sizeof *c->images);
+
+  /* Add IMG to c->images, and assign IMG an id.  */
+  c->images[i] = img;
+  img->id = i;
+  if (i == c->used)
+    ++c->used;
+
+  /* Add IMG to the cache's hash table.  */
+  i = img->hash % IMAGE_CACHE_BUCKETS_SIZE;
+  img->next = c->buckets[i];
+  if (img->next)
+    img->next->prev = img;
+  img->prev = NULL;
+  c->buckets[i] = img;
+}
+
 
 #if defined (HAVE_WEBP) || defined (HAVE_GIF)
 
@@ -12793,11 +12925,14 @@ DEFUN ("image-cache-size", Fimage_cache_size, Simage_cache_size, 0, 0, 0,
 {
   Lisp_Object tail, frame;
   size_t total = 0;
+  struct frame *f;
 
   FOR_EACH_FRAME (tail, frame)
-    if (FRAME_WINDOW_P (XFRAME (frame)))
-      total += image_frame_cache_size (XFRAME (frame));
-
+    {
+      f = XFRAME (frame);
+      if (FRAME_WINDOW_P (f))
+	total += image_frame_cache_size (FRAME_IMAGE_CACHE (f));
+    }
 #if defined (HAVE_WEBP) || defined (HAVE_GIF)
   struct anim_cache *pcache = anim_cache;
   while (pcache)
@@ -12810,6 +12945,24 @@ DEFUN ("image-cache-size", Fimage_cache_size, Simage_cache_size, 0, 0, 0,
   return make_int (total);
 }
 
+DEFUN ("user-image-cache-size", Fuser_image_cache_size, Suser_image_cache_size, 0, 0, 0,
+       doc: /* Return the size of the user's image cache.  */)
+  (void)
+{
+  Lisp_Object tail, frame;
+  size_t total = 0;
+  struct frame *f;
+
+  FOR_EACH_FRAME (tail, frame)
+    {
+      f = XFRAME (frame);
+      if (FRAME_WINDOW_P (f))
+	total += image_frame_cache_size (FRAME_USER_IMAGE_CACHE (f));
+    }
+
+  return make_int (total);
+}
+
 
 DEFUN ("init-image-library", Finit_image_library, Sinit_image_library, 1, 1, 0,
        doc: /* Initialize image library implementing image type TYPE.
@@ -12979,6 +13132,7 @@ syms_of_image (void)
   DEFSYM (QCcolor_adjustment, ":color-adjustment");
   DEFSYM (QCmask, ":mask");
   DEFSYM (QCflip, ":flip");
+  DEFSYM (QCttl, ":ttl");
 
   /* Other symbols.  */
   DEFSYM (Qlaplace, "laplace");
@@ -13142,6 +13296,7 @@ syms_of_image (void)
   defsubr (&Simage_mask_p);
   defsubr (&Simage_metadata);
   defsubr (&Simage_cache_size);
+  defsubr (&Suser_image_cache_size);
   defsubr (&Simagep);
 
 #ifdef GLYPH_DEBUG
-- 
2.46.2


[-- Attachment #3: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
> Date: Thu, 17 Oct 2024 11:51:08 +0200
> 
> I'm trying to revisit this idea of having a user controlled image cache.
> The goal is to have, for instance, image-mode use it because (hopefully)
> it would have a better idea of when to cache/uncache images for its
> usage.
> 
> Here is a patch of such a prototype « user image cache ».
> 
> For the moment, I wanted to keep it minimal so the only interface to use
> it goes as follow.  When a user (or a program) use the image attribute
> ":ttl TIME_IN_SECONDS" with `create-image' such image will be stored in
> the « user cache » and not in the internal one.  Images lookups use both
> caches (internal and internal, in that order).  A negative
> TIME_IN_SECONDS means « cache this image forever ».

Why do we need a separate cache for this? couldn't we instead just add
the EOL field to the single cache we already have?  having two caches
requires to search both of them, which slows down image look up, so if
we can avoid that, it's better.

> I think there will be a need for user function that completely flushes
> the images in this new cache.  But do you think we need others commands
> for such interface?

We probably need a way for modifying the time an image is to be kept,
at least.

Thanks.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> For the moment, I wanted to keep it minimal so the only interface to use
>> it goes as follow.  When a user (or a program) use the image attribute
>> ":ttl TIME_IN_SECONDS" with `create-image' such image will be stored in
>> the « user cache » and not in the internal one.  Images lookups use both
>> caches (internal and internal, in that order).  A negative
>> TIME_IN_SECONDS means « cache this image forever ».
>
> Why do we need a separate cache for this? couldn't we instead just add
> the EOL field to the single cache we already have?  having two caches
> requires to search both of them, which slows down image look up, so if
> we can avoid that, it's better.

That's you (in this thread) that suggested having another cache for more
"user controlled" usage and suggested that lookup_image could search in
both caches.

>> I think there will be a need for user function that completely flushes
>> the images in this new cache.  But do you think we need others commands
>> for such interface?
>
> We probably need a way for modifying the time an image is to be kept,
> at least.

Yes.  Anyway, my last patch was completely wrong and I am working on
another one.  Tell me if you're interested to see where I'm headed to.
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
> Date: Mon, 21 Oct 2024 12:12:12 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> >> For the moment, I wanted to keep it minimal so the only interface to use
> >> it goes as follow.  When a user (or a program) use the image attribute
> >> ":ttl TIME_IN_SECONDS" with `create-image' such image will be stored in
> >> the « user cache » and not in the internal one.  Images lookups use both
> >> caches (internal and internal, in that order).  A negative
> >> TIME_IN_SECONDS means « cache this image forever ».
> >
> > Why do we need a separate cache for this? couldn't we instead just add
> > the EOL field to the single cache we already have?  having two caches
> > requires to search both of them, which slows down image look up, so if
> > we can avoid that, it's better.
> 
> That's you (in this thread) that suggested having another cache for more
> "user controlled" usage and suggested that lookup_image could search in
> both caches.

I said that assuming the difference will be more prominent than just
one field.

The implementation could be a single cache with a field or fields that
distinguish between the two kinds of cached images.

> >> I think there will be a need for user function that completely flushes
> >> the images in this new cache.  But do you think we need others commands
> >> for such interface?
> >
> > We probably need a way for modifying the time an image is to be kept,
> > at least.
> 
> Yes.  Anyway, my last patch was completely wrong and I am working on
> another one.  Tell me if you're interested to see where I'm headed to.

It's your call.  I and others are here to provide feedback if you feel
you need that.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
>> Date: Mon, 21 Oct 2024 12:12:12 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> [...]
>> 
>> >> For the moment, I wanted to keep it minimal so the only interface to use
>> >> it goes as follow.  When a user (or a program) use the image attribute
>> >> ":ttl TIME_IN_SECONDS" with `create-image' such image will be stored in
>> >> the « user cache » and not in the internal one.  Images lookups use both
>> >> caches (internal and internal, in that order).  A negative
>> >> TIME_IN_SECONDS means « cache this image forever ».
>> >
>> > Why do we need a separate cache for this? couldn't we instead just add
>> > the EOL field to the single cache we already have?  having two caches
>> > requires to search both of them, which slows down image look up, so if
>> > we can avoid that, it's better.
>> 
>> That's you (in this thread) that suggested having another cache for more
>> "user controlled" usage and suggested that lookup_image could search in
>> both caches.
>
> I said that assuming the difference will be more prominent than just
> one field.
>
> The implementation could be a single cache with a field or fields that
> distinguish between the two kinds of cached images.

Yes, that makes sense.

>> >> I think there will be a need for user function that completely flushes
>> >> the images in this new cache.  But do you think we need others commands
>> >> for such interface?
>> >
>> > We probably need a way for modifying the time an image is to be kept,
>> > at least.
>> 
>> Yes.  Anyway, my last patch was completely wrong and I am working on
>> another one.  Tell me if you're interested to see where I'm headed to.
>
> It's your call.  I and others are here to provide feedback if you feel
> you need that.

Anyway, here is a patch of what I have done.

Since in the end an image is identify by an ID which is its index in the
images vector, I choose to take this vector out of the cache struct and
put it into what I called an image_store.

Now there are two caches (internal (as before) and user's) that both use
the image_store.  In order to make this work, I needed to add a
reference to the cache that manage it in each image.

But, as you said, for just one other field, this code might be
over-engineered and also quite error-prone.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-User-controlled-image-cache.patch --]
[-- Type: text/x-patch, Size: 24479 bytes --]

From 295a9501067511bf82d48f273c88b9b9f94f483f Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Thu, 17 Oct 2024 11:27:56 +0200
Subject: [PATCH] User controlled image cache

---
 src/alloc.c      |   3 +-
 src/dispextern.h |  31 +++--
 src/frame.h      |  16 ++-
 src/image.c      | 310 ++++++++++++++++++++++++++++++++++++-----------
 src/xfaces.c     |  19 +++
 5 files changed, 290 insertions(+), 89 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 4fab0d54248..eee76fa11ce 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -7042,8 +7042,7 @@ mark_frame (struct Lisp_Vector *ptr)
 #ifdef HAVE_WINDOW_SYSTEM
   /* Mark this frame's image cache, though it might be common to several
      frames with the same font size.  */
-  if (FRAME_IMAGE_CACHE (f))
-    mark_image_cache (FRAME_IMAGE_CACHE (f));
+  mark_image_store (f);
 #endif /* HAVE_WINDOW_SYSTEM */
 }
 
diff --git a/src/dispextern.h b/src/dispextern.h
index cc248a4472e..63445772509 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3141,6 +3141,12 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
      in prepare_image_for_display.  */
   struct timespec timestamp;
 
+  /* The end of life for an image in the user's image cache.  */
+  struct timespec eol;
+
+  /* Link to the cache used for this image.  */
+  struct image_cache *cache;
+
   /* Pixmaps of the image.  */
   Emacs_Pixmap pixmap, mask;
 
@@ -3277,23 +3283,25 @@ #define CENTERED_IMAGE_ASCENT -1
   struct image *next, *prev;
 };
 
-
-/* Cache of images.  Each frame has a cache.  X frames with the same
-   x_display_info share their caches.  */
-
-struct image_cache
+struct image_store
 {
-  /* Hash table of images.  */
-  struct image **buckets;
-
   /* Vector mapping image ids to images.  */
   struct image **images;
 
   /* Allocated size of `images'.  */
   ptrdiff_t size;
 
-  /* Number of images in the cache.  */
+  /* Number of images in the store.  */
   ptrdiff_t used;
+};
+
+/* Cache of images.  Each frame has a cache.  X frames with the same
+   x_display_info share their caches.  */
+
+struct image_cache
+{
+  /* Hash table of images.  */
+  struct image **buckets;
 
   /* Reference count (number of frames sharing this cache).  */
   ptrdiff_t refcount;
@@ -3632,6 +3640,7 @@ #define TRY_WINDOW_IGNORE_FONTS_CHANGE	(1 << 1)
 #ifdef HAVE_WINDOW_SYSTEM
 
 extern void clear_image_cache (struct frame *, Lisp_Object);
+extern void clear_user_image_cache (struct frame *);
 extern ptrdiff_t image_bitmap_pixmap (struct frame *, ptrdiff_t);
 extern void image_reference_bitmap (struct frame *, ptrdiff_t);
 extern ptrdiff_t image_create_bitmap_from_data (struct frame *, char *,
@@ -3652,10 +3661,11 @@ #define TRY_WINDOW_IGNORE_FONTS_CHANGE	(1 << 1)
 #endif
 extern Lisp_Object image_find_image_file (Lisp_Object);
 
+struct image_store *make_image_store (void);
 struct image_cache *make_image_cache (void);
 extern void free_image_cache (struct frame *);
 void clear_image_caches (Lisp_Object);
-void mark_image_cache (struct image_cache *);
+void mark_image_store (struct frame *);
 void image_prune_animation_caches (bool);
 bool valid_image_p (Lisp_Object);
 void prepare_image_for_display (struct frame *, struct image *);
@@ -3719,6 +3729,7 @@ #define RGB_PIXEL_COLOR COLORREF
 int lookup_derived_face (struct window *, struct frame *,
                          Lisp_Object, int, bool);
 #ifdef HAVE_WINDOW_SYSTEM
+extern struct image_store *share_image_store (struct frame *f);
 extern struct image_cache *share_image_cache (struct frame *f);
 #endif /* HAVE_WINDOW_SYSTEM */
 void init_frame_faces (struct frame *);
diff --git a/src/frame.h b/src/frame.h
index 1d920d1a6bc..6ded69ef14f 100644
--- a/src/frame.h
+++ b/src/frame.h
@@ -292,6 +292,12 @@ #define EMACS_FRAME_H
   /* Cache of realized images, which may be shared with other
      frames.  */
   struct image_cache *image_cache;
+
+  /* Another image cache that can be managed from Emacs. */
+  struct image_cache *user_image_cache;
+
+  /* Vector of cached images.  Used by internal and user's cache.  */
+  struct image_store *image_store;
 #endif /* HAVE_WINDOW_SYSTEM */
 
   /* Tab-bar item index of the item on which a mouse button was pressed.  */
@@ -916,6 +922,8 @@ #define FRAME_KBOARD(f) ((f)->terminal->kboard)
 
 /* Return a pointer to the image cache of frame F.  */
 #define FRAME_IMAGE_CACHE(F) ((F)->image_cache)
+#define FRAME_USER_IMAGE_CACHE(F) ((F)->user_image_cache)
+#define FRAME_IMAGE_STORE(F) ((F)->image_store)
 
 #define XFRAME(p) \
   (eassert (FRAMEP (p)), XUNTAG (p, Lisp_Vectorlike, struct frame))
@@ -1668,8 +1676,8 @@ FACE_FROM_ID_OR_NULL (struct frame *f, int id)
 INLINE struct image *
 IMAGE_FROM_ID (struct frame *f, int id)
 {
-  eassert (0 <= id && id < FRAME_IMAGE_CACHE (f)->used);
-  return FRAME_IMAGE_CACHE (f)->images[id];
+  eassert (0 <= id && id < FRAME_IMAGE_STORE (f)->used);
+  return FRAME_IMAGE_STORE (f)->images[id];
 }
 
 /* Value is a pointer to the image with id ID on frame F, or null if
@@ -1678,9 +1686,9 @@ IMAGE_FROM_ID (struct frame *f, int id)
 INLINE struct image *
 IMAGE_OPT_FROM_ID (struct frame *f, int id)
 {
-  int used = FRAME_IMAGE_CACHE (f)->used;
+  int used = FRAME_IMAGE_STORE (f)->used;
   eassume (0 <= used);
-  return 0 <= id && id < used ? FRAME_IMAGE_CACHE (f)->images[id] : NULL;
+  return 0 <= id && id < used ? FRAME_IMAGE_STORE (f)->images[id] : NULL;
 }
 #endif
 \f
diff --git a/src/image.c b/src/image.c
index 34936977a40..7d6ad18bd51 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1744,6 +1744,7 @@ make_image (Lisp_Object spec, EMACS_UINT hash)
 {
   struct image *img = xzalloc (sizeof *img);
   Lisp_Object file = image_spec_value (spec, QCfile, NULL);
+  Lisp_Object ttl = image_spec_value (spec, QCttl, NULL);
 
   eassert (valid_image_p (spec));
   img->dependencies = NILP (file) ? Qnil : list1 (file);
@@ -1754,18 +1755,36 @@ make_image (Lisp_Object spec, EMACS_UINT hash)
   img->ascent = DEFAULT_IMAGE_ASCENT;
   img->hash = hash;
   img->corners[BOT_CORNER] = -1;  /* Full image */
+  if (! NILP (ttl) && FIXNUMP (ttl))
+    {
+      struct timespec now = current_timespec ();
+      struct timespec ttl_spec;
+
+      /* A negative TTL is used to mean forever.  */
+      if (XFIXNUM (ttl) < 0)
+	(img->eol).tv_sec = -1;
+      else
+	{
+	  ttl_spec.tv_sec = XFIXNUM (ttl);
+	  ttl_spec.tv_nsec = 0;
+	  img->eol = timespec_add(now, ttl_spec);
+	}
+    }
+  else
+    (img->eol).tv_sec = (img->eol).tv_nsec = 0;
+
   return img;
 }
 
 
-/* Free image IMG which was used on frame F, including its resources.  */
+/* Free image IMG from the cache C, including its resources on frame F.  */
 
 static void
-free_image (struct frame *f, struct image *img)
+free_image (struct image_cache *c, struct frame *f, struct image *img)
 {
   if (img)
     {
-      struct image_cache *c = FRAME_IMAGE_CACHE (f);
+      struct image_store *s = FRAME_IMAGE_STORE (f);
 
       /* Remove IMG from the hash table of its cache.  */
       if (img->prev)
@@ -1776,7 +1795,8 @@ free_image (struct frame *f, struct image *img)
       if (img->next)
 	img->next->prev = img->prev;
 
-      c->images[img->id] = NULL;
+      /* Free store's slot.  */
+      s->images[img->id] = NULL;
 
 #if !defined USE_CAIRO && defined HAVE_XRENDER
       /* FRAME_X_DISPLAY (f) could be NULL if this is being called from
@@ -2178,7 +2198,20 @@ image_alloc_image_color (struct frame *f, struct image *img,
 			     Image Cache
  ***********************************************************************/
 
+static void do_cache_image (struct image_cache *, struct frame *, struct image *);
 static void cache_image (struct frame *f, struct image *img);
+static void user_cache_image (struct frame *f, struct image *img);
+
+struct image_store *
+make_image_store (void)
+{
+  struct image_store *s = xmalloc (sizeof *s);
+
+  s->size = 50;
+  s->used = 0;
+  s->images = xmalloc (s->size * sizeof *s->images);
+  return s;
+}
 
 /* Return a new, initialized image cache that is allocated from the
    heap.  Call free_image_cache to free an image cache.  */
@@ -2188,25 +2221,22 @@ make_image_cache (void)
 {
   struct image_cache *c = xmalloc (sizeof *c);
 
-  c->size = 50;
-  c->used = c->refcount = 0;
-  c->images = xmalloc (c->size * sizeof *c->images);
+  c->refcount = 0;
   c->buckets = xzalloc (IMAGE_CACHE_BUCKETS_SIZE * sizeof *c->buckets);
   /* This value should never be encountered.  */
   c->scaling_col_width = -1;
   return c;
 }
 
-/* Find an image matching SPEC in the cache, and return it.  If no
+/* Find an image matching SPEC in the cache C, and return it.  If no
    image is found, return NULL.  */
 static struct image *
-search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
+search_image_cache (struct image_cache *c, Lisp_Object spec, EMACS_UINT hash,
                     unsigned long foreground, unsigned long background,
                     int font_size, char *font_family, bool ignore_colors)
 {
   struct image *img;
-  struct image_cache *c = FRAME_IMAGE_CACHE (f);
-  int i = hash % IMAGE_CACHE_BUCKETS_SIZE;
+  int i;
 
   if (!c) return NULL;
 
@@ -2222,6 +2252,7 @@ search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
      image spec specifies :background.  However, the extra memory
      usage is probably negligible in practice, so we don't bother.  */
 
+  i = hash % IMAGE_CACHE_BUCKETS_SIZE;
   for (img = c->buckets[i]; img; img = img->next)
     if (img->hash == hash
 	&& !NILP (Fequal (img->spec, spec))
@@ -2258,11 +2289,13 @@ filter_image_spec (Lisp_Object spec)
 	  spec = XCDR (spec);
 
 	  /* Some animation-related data doesn't affect display, but
-	     breaks the image cache.  Filter those out.  */
+	     breaks the image cache.  Filter those out.  Also filter out
+	     the user's cache time to live.  */
 	  if (!(EQ (key, QCanimate_buffer)
 		|| EQ (key, QCanimate_tardiness)
 		|| EQ (key, QCanimate_position)
-		|| EQ (key, QCanimate_multi_frame_data)))
+		|| EQ (key, QCanimate_multi_frame_data)
+		|| EQ (key, QCttl)))
 	    {
 	      out = Fcons (value, out);
 	      out = Fcons (key, out);
@@ -2279,14 +2312,15 @@ uncache_image (struct frame *f, Lisp_Object spec)
 {
   struct image *img;
   EMACS_UINT hash = sxhash (filter_image_spec (spec));
+  struct image_cache *c = FRAME_IMAGE_CACHE (f);
 
   /* Because the background colors are based on the current face, we
      can have multiple copies of an image with the same spec. We want
      to remove them all to ensure the user doesn't see an old version
      of the image when the face changes.  */
-  while ((img = search_image_cache (f, spec, hash, 0, 0, 0, NULL, true)))
+  while ((img = search_image_cache (c, spec, hash, 0, 0, 0, NULL, true)))
     {
-      free_image (f, img);
+      free_image (c, f, img);
       /* As display glyphs may still be referring to the image ID, we
 	 must garbage the frame (Bug#6426).  */
       SET_FRAME_GARBAGED (f);
@@ -2301,6 +2335,7 @@ uncache_image (struct frame *f, Lisp_Object spec)
 free_image_cache (struct frame *f)
 {
   struct image_cache *c = FRAME_IMAGE_CACHE (f);
+  struct image_store *s = FRAME_IMAGE_STORE (f);
   ptrdiff_t i;
 
   /* This function assumes the caller already verified that the frame's
@@ -2309,9 +2344,8 @@ free_image_cache (struct frame *f)
   /* Cache should not be referenced by any frame when freed.  */
   eassert (c->refcount == 0);
 
-  for (i = 0; i < c->used; ++i)
-    free_image (f, c->images[i]);
-  xfree (c->images);
+  for (i = 0; i < s->used; ++i)
+      free_image (c, f, s->images[i]);
   xfree (c->buckets);
   xfree (c);
 }
@@ -2328,8 +2362,9 @@ free_image_cache (struct frame *f)
 clear_image_cache (struct frame *f, Lisp_Object filter)
 {
   struct image_cache *c = FRAME_IMAGE_CACHE (f);
+  struct image_store *s = FRAME_IMAGE_STORE (f);
 
-  if (c && !f->inhibit_clear_image_cache)
+  if (c && s && !f->inhibit_clear_image_cache)
     {
       ptrdiff_t i, nfreed = 0;
 
@@ -2340,13 +2375,15 @@ clear_image_cache (struct frame *f, Lisp_Object filter)
       if (!NILP (filter))
 	{
 	  /* Filter image cache.  */
-	  for (i = 0; i < c->used; ++i)
+	  for (i = 0; i < s->used; ++i)
 	    {
-	      struct image *img = c->images[i];
-	      if (img && (EQ (Qt, filter)
-			  || !NILP (Fmember (filter, img->dependencies))))
+	      struct image *img = s->images[i];
+	      if (img && (img->cache == c)
+		  && (EQ (Qt, filter)
+		      || !NILP (Fmember (filter, img->dependencies))))
 		{
-		  free_image (f, img);
+		  free_image (c, f, img);
+		  s->images[i] = NULL;
 		  ++nfreed;
 		}
 	    }
@@ -2358,8 +2395,8 @@ clear_image_cache (struct frame *f, Lisp_Object filter)
 	  double delay;
 	  ptrdiff_t nimages = 0;
 
-	  for (i = 0; i < c->used; ++i)
-	    if (c->images[i])
+	  for (i = 0; i < s->used; ++i)
+	    if (s->images[i] && s->images[i]->cache == c)
 	      nimages++;
 
 	  /* If the number of cached images has grown unusually large,
@@ -2372,12 +2409,13 @@ clear_image_cache (struct frame *f, Lisp_Object filter)
 	  t = current_timespec ();
 	  old = timespec_sub (t, dtotimespec (delay));
 
-	  for (i = 0; i < c->used; ++i)
+	  for (i = 0; i < s->used; ++i)
 	    {
-	      struct image *img = c->images[i];
-	      if (img && timespec_cmp (img->timestamp, old) < 0)
+	      struct image *img = s->images[i];
+	      if (img && img->cache == c
+		  && timespec_cmp (img->timestamp, old) < 0)
 		{
-		  free_image (f, img);
+		  free_image (c, f, img);
 		  ++nfreed;
 		}
 	    }
@@ -2405,6 +2443,57 @@ clear_image_cache (struct frame *f, Lisp_Object filter)
     }
 }
 
+void
+clear_user_image_cache (struct frame *f)
+{
+  struct image_cache *c = FRAME_USER_IMAGE_CACHE (f);
+  struct image_store *s = FRAME_IMAGE_STORE (f);
+
+  if (c && s && !f->inhibit_clear_image_cache)
+    {
+      ptrdiff_t i, nfreed = 0;
+      struct timespec t = current_timespec ();
+
+      /* Block input so that we won't be interrupted by a SIGIO
+	 while being in an inconsistent state.  */
+      block_input ();
+
+      /* Filter image cache based on image's time to live.  */
+      for (i = 0; i < s->used; ++i)
+	{
+	  struct image *img = s->images[i];
+	  if (img && img->cache == c)
+	    {
+	      if (((img->eol).tv_sec >= 0) && timespec_cmp (t, img->eol) > 0)
+		{
+		  free_image (c, f, img);
+		  ++nfreed;
+		}
+	    }
+	}
+
+      /* We may be clearing the image cache because, for example,
+	 Emacs was iconified for a longer period of time.  In that
+	 case, current matrices may still contain references to
+	 images freed above.  So, clear these matrices.  */
+      if (nfreed)
+	{
+	  Lisp_Object tail, frame;
+
+	  FOR_EACH_FRAME (tail, frame)
+	    {
+	      struct frame *fr = XFRAME (frame);
+	      if (FRAME_USER_IMAGE_CACHE (fr) == c)
+		clear_current_matrices (fr);
+	    }
+
+	  windows_or_buffers_changed = 19;
+	}
+
+      unblock_input ();
+    }
+}
+
 void
 clear_image_caches (Lisp_Object filter)
 {
@@ -2415,7 +2504,10 @@ clear_image_caches (Lisp_Object filter)
   Lisp_Object tail, frame;
   FOR_EACH_FRAME (tail, frame)
     if (FRAME_WINDOW_P (XFRAME (frame)))
-      clear_image_cache (XFRAME (frame), filter);
+      {
+	clear_image_cache (XFRAME (frame), filter);
+	clear_user_image_cache (XFRAME (frame));
+      }
 }
 
 DEFUN ("clear-image-cache", Fclear_image_cache, Sclear_image_cache,
@@ -2444,7 +2536,11 @@ DEFUN ("clear-image-cache", Fclear_image_cache, Sclear_image_cache,
   if (! (NILP (filter) || FRAMEP (filter)))
     clear_image_caches (filter);
   else
-    clear_image_cache (decode_window_system_frame (filter), Qt);
+    {
+      struct frame *decoded_frame = decode_window_system_frame (filter);
+      clear_image_cache (decoded_frame, Qt);
+      clear_user_image_cache (decoded_frame);
+    }
 
   /* Also clear the animation caches.  */
   image_prune_animation_caches (true);
@@ -2501,17 +2597,18 @@ image_size_in_bytes (struct image *img)
 }
 
 static size_t
-image_frame_cache_size (struct frame *f)
+image_frame_cache_size (struct image_cache *c, struct frame *f)
 {
-  struct image_cache *c = FRAME_IMAGE_CACHE (f);
   if (!c)
     return 0;
 
+  struct image_store *s = FRAME_IMAGE_STORE (f);
   size_t total = 0;
-  for (ptrdiff_t i = 0; i < c->used; ++i)
+  for (ptrdiff_t i = 0; i < s->used; ++i)
     {
-      struct image *img = c->images[i];
-      total += img ? image_size_in_bytes (img) : 0;
+      struct image *img = s->images[i];
+      if (img && img->cache == c)
+	total += image_size_in_bytes (img);
     }
   return total;
 }
@@ -3495,6 +3592,8 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
   unsigned long background = face->background;
   int font_size = face->font->pixel_size;
   char *font_family = SSDATA (face->lface[LFACE_FAMILY_INDEX]);
+  struct image_cache *c;
+  Lisp_Object ttl = image_spec_value (spec, QCttl, NULL);
 
   /* F must be a window-system frame, and SPEC must be a valid image
      specification.  */
@@ -3503,11 +3602,17 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
 
   /* Look up SPEC in the hash table of the image cache.  */
   hash = sxhash (filter_image_spec (spec));
-  img = search_image_cache (f, spec, hash, foreground, background,
+  if (NILP (ttl))
+    c = FRAME_IMAGE_CACHE (f);
+  else /* SPEC as a ttl so lookup into the user's cache.  */
+    c = FRAME_USER_IMAGE_CACHE (f);
+
+  img = search_image_cache (c, spec, hash, foreground, background,
 			    font_size, font_family, false);
+
   if (img && img->load_failed_p)
     {
-      free_image (f, img);
+      free_image (c, f, img);
       img = NULL;
     }
 
@@ -3516,7 +3621,12 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
     {
       block_input ();
       img = make_image (spec, hash);
-      cache_image (f, img);
+      /* If image's end of life is set store it in the user's image
+	 cache instead.  */
+      if ((img->eol).tv_sec != 0)
+	user_cache_image (f, img);
+      else
+	cache_image (f, img);
       img->face_foreground = foreground;
       img->face_background = background;
       img->face_font_size = font_size;
@@ -3608,6 +3718,37 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
   return img->id;
 }
 
+void
+do_cache_image (struct image_cache *c, struct frame *f,
+		struct image *img)
+{
+  struct image_store *s = FRAME_IMAGE_STORE (f);
+  ptrdiff_t i;
+
+  /* Find a free slot in the store.  */
+  for (i = 0; i < s->used; ++i)
+    if (s->images[i] == NULL)
+      break;
+
+  /* If no free slot found, maybe enlarge s->images.  */
+  if (i == s->used && s->used == s->size)
+    s->images = xpalloc (s->images, &s->size, 1, -1, sizeof *s->images);
+
+  /* Add IMG to s->images, and assign IMG an id.  */
+  s->images[i] = img;
+  img->id = i;
+  if (i == s->used)
+    ++s->used;
+
+  /* Add IMG to the cache's hash table.  */
+  i = img->hash % IMAGE_CACHE_BUCKETS_SIZE;
+  img->next = c->buckets[i];
+  if (img->next)
+    img->next->prev = img;
+  img->prev = NULL;
+  c->buckets[i] = img;
+  img->cache = c;
+}
 
 /* Cache image IMG in the image cache of frame F.  */
 
@@ -3615,7 +3756,9 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
 cache_image (struct frame *f, struct image *img)
 {
   struct image_cache *c = FRAME_IMAGE_CACHE (f);
-  ptrdiff_t i;
+
+  if (!FRAME_IMAGE_STORE (f))
+    FRAME_IMAGE_STORE (f) = share_image_store (f);
 
   if (!c)
     {
@@ -3623,28 +3766,24 @@ cache_image (struct frame *f, struct image *img)
       c->refcount++;
     }
 
-  /* Find a free slot in c->images.  */
-  for (i = 0; i < c->used; ++i)
-    if (c->images[i] == NULL)
-      break;
+  do_cache_image (c, f, img);
+}
 
-  /* If no free slot found, maybe enlarge c->images.  */
-  if (i == c->used && c->used == c->size)
-    c->images = xpalloc (c->images, &c->size, 1, -1, sizeof *c->images);
+static void
+user_cache_image (struct frame *f, struct image *img)
+{
+  struct image_cache *c = FRAME_USER_IMAGE_CACHE (f);
 
-  /* Add IMG to c->images, and assign IMG an id.  */
-  c->images[i] = img;
-  img->id = i;
-  if (i == c->used)
-    ++c->used;
+  if (!FRAME_IMAGE_STORE (f))
+    FRAME_IMAGE_STORE (f) = share_image_store (f);
 
-  /* Add IMG to the cache's hash table.  */
-  i = img->hash % IMAGE_CACHE_BUCKETS_SIZE;
-  img->next = c->buckets[i];
-  if (img->next)
-    img->next->prev = img;
-  img->prev = NULL;
-  c->buckets[i] = img;
+  if (!c)
+    {
+      c = FRAME_USER_IMAGE_CACHE (f) = make_image_cache ();
+      c->refcount++;
+    }
+
+  do_cache_image (c, f, img);
 }
 
 
@@ -3762,14 +3901,16 @@ mark_image (struct image *img)
 
 
 void
-mark_image_cache (struct image_cache *c)
+mark_image_store (struct frame *f)
 {
-  if (c)
+  struct image_store *s = FRAME_IMAGE_STORE (f);
+
+  if (s)
     {
       ptrdiff_t i;
-      for (i = 0; i < c->used; ++i)
-	if (c->images[i])
-	  mark_image (c->images[i]);
+      for (i = 0; i < s->used; ++i)
+	if (s->images[i])
+	  mark_image (s->images[i]);
     }
 
 #if defined HAVE_WEBP || defined HAVE_GIF
@@ -12647,23 +12788,23 @@ gs_load (struct frame *f, struct image *img)
 void
 x_kill_gs_process (Pixmap pixmap, struct frame *f)
 {
-  struct image_cache *c = FRAME_IMAGE_CACHE (f);
+  struct image_store *s = FRAME_IMAGE_STORE (f);
   ptrdiff_t i;
   struct image *img;
 
   /* Find the image containing PIXMAP.  */
-  for (i = 0; i < c->used; ++i)
-    if (c->images[i]->pixmap == pixmap)
+  for (i = 0; i < s->used; ++i)
+    if (s->images[i]->pixmap == pixmap)
       break;
 
   /* Should someone in between have cleared the image cache, for
      instance, give up.  */
-  if (i == c->used)
+  if (i == s->used)
     return;
 
   /* Kill the GS process.  We should have found PIXMAP in the image
      cache and its image should contain a process object.  */
-  img = c->images[i];
+  img = s->images[i];
   eassert (PROCESSP (img->lisp_data));
   Fkill_process (img->lisp_data, Qnil);
   img->lisp_data = Qnil;
@@ -12793,11 +12934,14 @@ DEFUN ("image-cache-size", Fimage_cache_size, Simage_cache_size, 0, 0, 0,
 {
   Lisp_Object tail, frame;
   size_t total = 0;
+  struct frame *f;
 
   FOR_EACH_FRAME (tail, frame)
-    if (FRAME_WINDOW_P (XFRAME (frame)))
-      total += image_frame_cache_size (XFRAME (frame));
-
+    {
+      f = XFRAME (frame);
+      if (FRAME_WINDOW_P (f))
+	total += image_frame_cache_size (FRAME_IMAGE_CACHE (f), f);
+    }
 #if defined (HAVE_WEBP) || defined (HAVE_GIF)
   struct anim_cache *pcache = anim_cache;
   while (pcache)
@@ -12810,6 +12954,24 @@ DEFUN ("image-cache-size", Fimage_cache_size, Simage_cache_size, 0, 0, 0,
   return make_int (total);
 }
 
+DEFUN ("user-image-cache-size", Fuser_image_cache_size, Suser_image_cache_size, 0, 0, 0,
+       doc: /* Return the size of the user's image cache.  */)
+  (void)
+{
+  Lisp_Object tail, frame;
+  size_t total = 0;
+  struct frame *f;
+
+  FOR_EACH_FRAME (tail, frame)
+    {
+      f = XFRAME (frame);
+      if (FRAME_WINDOW_P (f))
+	total += image_frame_cache_size (FRAME_USER_IMAGE_CACHE (f), f);
+    }
+
+  return make_int (total);
+}
+
 
 DEFUN ("init-image-library", Finit_image_library, Sinit_image_library, 1, 1, 0,
        doc: /* Initialize image library implementing image type TYPE.
@@ -12979,6 +13141,7 @@ syms_of_image (void)
   DEFSYM (QCcolor_adjustment, ":color-adjustment");
   DEFSYM (QCmask, ":mask");
   DEFSYM (QCflip, ":flip");
+  DEFSYM (QCttl, ":ttl");
 
   /* Other symbols.  */
   DEFSYM (Qlaplace, "laplace");
@@ -13142,6 +13305,7 @@ syms_of_image (void)
   defsubr (&Simage_mask_p);
   defsubr (&Simage_metadata);
   defsubr (&Simage_cache_size);
+  defsubr (&Suser_image_cache_size);
   defsubr (&Simagep);
 
 #ifdef GLYPH_DEBUG
diff --git a/src/xfaces.c b/src/xfaces.c
index e248279e9b7..bea592fe7a8 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -636,6 +636,25 @@ x_free_gc (struct frame *f, struct android_gc *gc)
 
 #ifdef HAVE_WINDOW_SYSTEM
 
+struct image_store *
+share_image_store (struct frame *f)
+{
+  Lisp_Object tail, frame;
+  struct image_store *store;
+
+  FOR_EACH_FRAME (tail, frame)
+    {
+      struct frame *x = XFRAME (frame);
+
+      if (FRAME_TERMINAL (x) == FRAME_TERMINAL (f)
+	  && FRAME_IMAGE_STORE (x))
+	return FRAME_IMAGE_STORE (x);
+    }
+
+  store = make_image_store ();
+  return store;
+}
+
 /* Find an existing image cache registered for a frame on F's display
    and with a `scaling_col_width' of F's FRAME_COLUMN_WIDTH, or, in the
    absence of an eligible image cache, allocate an image cache with the
-- 
2.46.2


[-- Attachment #3: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
> Date: Mon, 21 Oct 2024 16:25:22 +0200
> 
> >> > We probably need a way for modifying the time an image is to be kept,
> >> > at least.
> >> 
> >> Yes.  Anyway, my last patch was completely wrong and I am working on
> >> another one.  Tell me if you're interested to see where I'm headed to.
> >
> > It's your call.  I and others are here to provide feedback if you feel
> > you need that.
> 
> Anyway, here is a patch of what I have done.
> 
> Since in the end an image is identify by an ID which is its index in the
> images vector, I choose to take this vector out of the cache struct and
> put it into what I called an image_store.

What advantages did you see in doing it this way, wrt the original
code?

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
>> Date: Mon, 21 Oct 2024 16:25:22 +0200
>> 
>> >> > We probably need a way for modifying the time an image is to be kept,
>> >> > at least.
>> >> 
>> >> Yes.  Anyway, my last patch was completely wrong and I am working on
>> >> another one.  Tell me if you're interested to see where I'm headed to.
>> >
>> > It's your call.  I and others are here to provide feedback if you feel
>> > you need that.
>> 
>> Anyway, here is a patch of what I have done.
>> 
>> Since in the end an image is identify by an ID which is its index in the
>> images vector, I choose to take this vector out of the cache struct and
>> put it into what I called an image_store.
>
> What advantages did you see in doing it this way, wrt the original
> code?

I don't think there is any great advantage.  I think I was more guide to
doing it this way because I thought I need to make it into another
(different) cache.
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi Eli and Stefan,

Here is a new version for my patch to add some user control on the image
cache.  This time, I have done some tests and benchmarks.

Instead of trying to have a completely different cache, I'm using the
current one but I had a condition to flush (or not) an image based on a
user defined time to live (TTL) for an image.  This TTL is a number of
seconds since the image was last displayed.

I have also tested it in image-mode with the second patch.  I tested by
browsing a directory with some rather large pictures in it (4000x3000
pixels, ≈6MB for each file).  I opened the first image in a 1640x1000
window (so each image is scaled) and did the following:

  - M-: (dotimes (i 10) (image-next-file 1))  ;; for cache warming
  - M-: (dotimes (i 10) (image-next-file -1)) ;; return to first image
  - M-: (benchmark-run 10 (dotimes (i 10) (image-next-file 1))
                          (dotimes (i 10) (image-next-file -1)))

Here is the timings I get:

     - with master (ea685170063): (109.208767158 15 1.0938777159999997)
     - with this patch: (6.934307561 15 1.095243891)

WDYT?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-User-defined-time-to-live-for-image.patch --]
[-- Type: text/x-patch, Size: 3283 bytes --]

From cfa3858ab954fd8354d3b080975f2d679004f294 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Mon, 28 Oct 2024 14:35:06 +0100
Subject: [PATCH 1/2] User defined time to live for image

---
 src/dispextern.h |  3 +++
 src/image.c      | 33 +++++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index cc248a4472e..52887ce1687 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3141,6 +3141,9 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
      in prepare_image_for_display.  */
   struct timespec timestamp;
 
+  /* Time to live after last displayed in seconds.  */
+  EMACS_INT ttl;
+
   /* Pixmaps of the image.  */
   Emacs_Pixmap pixmap, mask;
 
diff --git a/src/image.c b/src/image.c
index 34936977a40..1343276b8c8 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1744,6 +1744,7 @@ make_image (Lisp_Object spec, EMACS_UINT hash)
 {
   struct image *img = xzalloc (sizeof *img);
   Lisp_Object file = image_spec_value (spec, QCfile, NULL);
+  Lisp_Object ttl = image_spec_value (spec, QCttl, NULL);
 
   eassert (valid_image_p (spec));
   img->dependencies = NILP (file) ? Qnil : list1 (file);
@@ -1754,6 +1755,11 @@ make_image (Lisp_Object spec, EMACS_UINT hash)
   img->ascent = DEFAULT_IMAGE_ASCENT;
   img->hash = hash;
   img->corners[BOT_CORNER] = -1;  /* Full image */
+  if (! NILP (ttl) && FIXNUMP (ttl))
+    img->ttl = XFIXNUM (ttl);
+  else
+    img->ttl = 0;
+
   return img;
 }
 
@@ -2258,11 +2264,13 @@ filter_image_spec (Lisp_Object spec)
 	  spec = XCDR (spec);
 
 	  /* Some animation-related data doesn't affect display, but
-	     breaks the image cache.  Filter those out.  */
+	     breaks the image cache.  Filter those out.  Also filter out
+	     the user's cache time to live.  */
 	  if (!(EQ (key, QCanimate_buffer)
 		|| EQ (key, QCanimate_tardiness)
 		|| EQ (key, QCanimate_position)
-		|| EQ (key, QCanimate_multi_frame_data)))
+		|| EQ (key, QCanimate_multi_frame_data)
+		|| EQ (key, QCttl)))
 	    {
 	      out = Fcons (value, out);
 	      out = Fcons (key, out);
@@ -2332,11 +2340,31 @@ clear_image_cache (struct frame *f, Lisp_Object filter)
   if (c && !f->inhibit_clear_image_cache)
     {
       ptrdiff_t i, nfreed = 0;
+      struct timespec now = current_timespec ();
+      struct timespec t, delay;
 
       /* Block input so that we won't be interrupted by a SIGIO
 	 while being in an inconsistent state.  */
       block_input ();
 
+      /* First, filter image cache based on image's time to live.  */
+      for (i = 0; i < c->used; ++i)
+	{
+	  struct image *img = c->images[i];
+	  /* A negative TTL means that this image is cached forever.  */
+	  if (img && img->ttl >= 0)
+	    {
+	      delay.tv_sec = img->ttl;
+	      delay.tv_nsec = 0;
+	      t = timespec_add(now, delay);
+	      if (timespec_cmp (t, img->timestamp) > 0)
+		{
+		  free_image (f, img);
+		  ++nfreed;
+		}
+	    }
+	}
+
       if (!NILP (filter))
 	{
 	  /* Filter image cache.  */
@@ -12979,6 +13007,7 @@ syms_of_image (void)
   DEFSYM (QCcolor_adjustment, ":color-adjustment");
   DEFSYM (QCmask, ":mask");
   DEFSYM (QCflip, ":flip");
+  DEFSYM (QCttl, ":ttl");
 
   /* Other symbols.  */
   DEFSYM (Qlaplace, "laplace");
-- 
2.47.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Try-it-in-image-mode.patch --]
[-- Type: text/x-patch, Size: 1587 bytes --]

From ca5df9e0e7cc00eea1e7fc74d41b33c52aeddfab Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Mon, 28 Oct 2024 14:37:47 +0100
Subject: [PATCH 2/2] Try it in image-mode

---
 lisp/image-mode.el | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lisp/image-mode.el b/lisp/image-mode.el
index e75f6ea918f..ffc2697085e 100644
--- a/lisp/image-mode.el
+++ b/lisp/image-mode.el
@@ -949,12 +949,14 @@ image-toggle-display-image
     ;; default scaling based on font size.
     (setq image (if (not edges)
 		    (create-image file-or-data type data-p :scale 1
-                                  :format (and filename data-p))
+                                  :format (and filename data-p)
+                                  :ttl 30)
 		  (create-image file-or-data type data-p :scale 1
 				:max-width max-width
 				:max-height max-height
                                 ;; Type hint.
-                                :format (and filename data-p))))
+                                :format (and filename data-p)
+                                :ttl 30)))
 
     ;; Handle `fit-window'.
     (when (and (eq image-transform-resize 'fit-window)
@@ -964,8 +966,6 @@ image-toggle-display-image
                   (plist-put (cdr image) :width
                              (plist-get (cdr image) :max-width)))))
 
-    ;; Discard any stale image data before looking it up again.
-    (image-flush image)
     (setq image (image--update-properties image (image-transform-properties image)))
     (setq props
 	  `(display ,image
-- 
2.47.0


[-- Attachment #4: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
> Date: Mon, 28 Oct 2024 15:31:22 +0100
> 
> Here is a new version for my patch to add some user control on the image
> cache.  This time, I have done some tests and benchmarks.
> 
> Instead of trying to have a completely different cache, I'm using the
> current one but I had a condition to flush (or not) an image based on a
> user defined time to live (TTL) for an image.  This TTL is a number of
> seconds since the image was last displayed.
> 
> I have also tested it in image-mode with the second patch.  I tested by
> browsing a directory with some rather large pictures in it (4000x3000
> pixels, ≈6MB for each file).  I opened the first image in a 1640x1000
> window (so each image is scaled) and did the following:
> 
>   - M-: (dotimes (i 10) (image-next-file 1))  ;; for cache warming
>   - M-: (dotimes (i 10) (image-next-file -1)) ;; return to first image
>   - M-: (benchmark-run 10 (dotimes (i 10) (image-next-file 1))
>                           (dotimes (i 10) (image-next-file -1)))
> 
> Here is the timings I get:
> 
>      - with master (ea685170063): (109.208767158 15 1.0938777159999997)
>      - with this patch: (6.934307561 15 1.095243891)

The C part looks OK, but I don't understand how will this be used by
Lisp programs.  We don't intend to use the new TTL member by default
each time we display an image, do we?  So I think we need to analyze
the various uses of images and decide which ones will use TTL (and
what values to use in that case) and which won't.

I also envision a need to have a Lisp API which takes an existing
image and sets its TTL to a given value.  And maybe other APIs as
well, I don't know.  But without such APIs and without sensible
defaults about them, this features sounds incomplete.

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: stefankangas@gmail.com,  68006@debbugs.gnu.org
>> Date: Mon, 28 Oct 2024 15:31:22 +0100
>> 
>> Here is a new version for my patch to add some user control on the image
>> cache.  This time, I have done some tests and benchmarks.
>> 
>> Instead of trying to have a completely different cache, I'm using the
>> current one but I had a condition to flush (or not) an image based on a
>> user defined time to live (TTL) for an image.  This TTL is a number of
>> seconds since the image was last displayed.
>> 
>> I have also tested it in image-mode with the second patch.  I tested by
>> browsing a directory with some rather large pictures in it (4000x3000
>> pixels, ≈6MB for each file).  I opened the first image in a 1640x1000
>> window (so each image is scaled) and did the following:
>> 
>>   - M-: (dotimes (i 10) (image-next-file 1))  ;; for cache warming
>>   - M-: (dotimes (i 10) (image-next-file -1)) ;; return to first image
>>   - M-: (benchmark-run 10 (dotimes (i 10) (image-next-file 1))
>>                           (dotimes (i 10) (image-next-file -1)))
>> 
>> Here is the timings I get:
>> 
>>      - with master (ea685170063): (109.208767158 15 1.0938777159999997)
>>      - with this patch: (6.934307561 15 1.095243891)
>
> The C part looks OK, but I don't understand how will this be used by
> Lisp programs.  We don't intend to use the new TTL member by default
> each time we display an image, do we?  So I think we need to analyze
> the various uses of images and decide which ones will use TTL (and
> what values to use in that case) and which won't.

Yes I thought that image-mode was a good candidate because I'm typically
using it on directory with lots of images that I'd like to browse (with
'n' and 'p').  But I imagine that this is not a use case for everybody.
In the same idea, I thought that DocView may also be a good candidate.

What I imagined (on the Lisp side) is that a program could load a subset
of images that are close to (in the sense of `image-next-file') the
current one with a TTL set.

> I also envision a need to have a Lisp API which takes an existing
> image and sets its TTL to a given value.  And maybe other APIs as
> well, I don't know.  But without such APIs and without sensible
> defaults about them, this features sounds incomplete.

Yes, sure there is a need for such Lisp API.  I'll try to come up with
something.
-- 
Manuel Giraud

bug#68006: 30.0.50; Image-mode speed

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefankangas@gmail.com> writes:

> Manuel Giraud via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs@gnu.org> writes:
>
>> And here is a new version of the patch.  Maybe, we need a NEWS entry for
>> this too.
>
> If we have an option, then yes we should add it to NEWS too.
>
> But I'm not sure about this one:
>
> Flushing the image cache all the time is fine for images that are 4KB in
> size, but these days we routinely work with images that are several
> megabytes in size (to give an idea, searching online tells me that
> Twitter allows uploading photos up to 2 MB, Facebook up to 15 MB).

Hi Stefan,

First, I want to say that here we're just talking about image-mode (and
its derivatives like docview) and how the image cache is managed in this
mode only.

"Flushing the image cache all the time" is what is done *now* in
image-mode.  In fact, this new option permits to inhibit it.

> Furthermore, my experience with image-dired has taught me that Emacs is
> terribly slow when compared to all other programs capable of viewing
> images out there.  So I think we should think a bit more about this
> one.

Yes and this slowness was what I wanted to speed up in the first place
in image-mode: moving from one image to the next is slower than any
other program.  I know that working on the image cache is picking a
low-hanging fruit here but I'm not an expert in fast image processing.

> Taking a step back, why are images treated differently from other
> buffers?  If the risk is that the image changes on disk without us
> noticing, then users should need to either run `revert-buffer' or enable
> `auto-revert-mode'.  If we are talking about images that are inline in a
> buffer, the cache should be flushed only when the buffer itself is
> reverted.  What am I missing?

I guess that makes sense but would it be easy to plug the
'revert-buffer' machinery into the image cache internals?  I don't know.

> Lastly, I'm not sure about the "hash the first 4 KB of an image"
> heuristic, for starters because images can be several megabytes in size.
> Would it not be both more accurate and faster to check the mtime and/or
> the size of the file?  Or do we need different heuristic for different
> image formats?  (Maybe JPEG changes the first 4 KB on save, but I don't
> think all bitmap formats do.)  But wouldn't it be even better to use the
> notify system when possible, like with `auto-revert-use-notify'?

You're right that this heuristic might not be the best one.  I
repurposed it from image-dired when Eli suggested something content
based.  Maybe we could have a modification time of the image into its
spec and compare it to said image file before display.

Best regards,
-- 
Manuel Giraud