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]

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