GNU bug report logs - #74537
[PATCH] An on-disk image modification does a cache miss

Previous Next

To reply to this bug, email your comments to 74537 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] An on-disk image modification does a cache miss
Date: Mon, 25 Nov 2024 22:24:25 +0100

[Message part 1 (text/plain, inline)]

Tags: patch

Hi,

With this patch, when an image file is modified on disk, the associated
cache image is invalidated and re-read from disk.

In GNU Emacs 31.0.50 (build 4, x86_64-unknown-openbsd7.6, X toolkit) of
 2024-11-23 built on computer
Repository revision: 5072b6110b69a8ba0d42772b26533dcdc39ffdfc
Repository branch: mgi/jpeg
Windowing system distributor 'The X.Org Foundation', version 11.0.12101014
System Description: OpenBSD computer 7.6 GENERIC.MP#437 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=lucid
 --with-toolkit-scroll-bars=no --without-cairo
 --without-compress-install'

[0001-An-on-disk-image-modification-does-a-cache-miss.patch (text/patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
Manuel Giraud

Message #8 received at 74537 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: 74537 <at> debbugs.gnu.org
Subject: Re: bug#74537: [PATCH] An on-disk image modification does a cache miss
Date: Tue, 26 Nov 2024 15:31:50 +0200

> Date: Mon, 25 Nov 2024 22:24:25 +0100
> From:  Manuel Giraud via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> With this patch, when an image file is modified on disk, the associated
> cache image is invalidated and re-read from disk.

Thanks, but I'm not sure it's a good idea to do that automatically
whenever lookup_image is called.

First, lookup_image is called only when we need to create or access an
image with only its Lisp descriptor in hand; once the image has been
displayed, that should happen relatively rarely.  For example, the
glyph matrices used to manipulate the display record only the image's
cached ID, and the code accesses the cached image without calling
lookup_image.  So this change could have weird confusing effects,
whereby the fact that the image was modified on disk becomes apparent
only after some display-related actions, but not after others.  For
example, scrolling a window by a small amount will not notice the
change on disk, whereas significant changes, especially when the image
goes off the window and back into it, will show the change.  Don't you
see such issues if you install the change?

Second, if the image is already on display, and the file changes on
disk, some layout calculations could see different dimensions than
what is actually on display, which will cause subtle bugs.  For
example, what if there's a 'display' property such as this:

  '(space :width (0.5 (image PROPS)))

and an image with those same PROPS is already shown on display?  The
code which implements the above calls lookup_image, which will
simulate a cache miss if the file has changed, and will then load the
new file, which could return dimensions different from the image
already on display.  So the width of the space glyph produced by the
above will be different from that of the displayed image, and we could
have misalignment until the next redisplay which redraws the image.

So I think this should probably be controlled by a variable visible
from Lisp, by default off, and we might also have a function to
invalidate all the cached images whose files have changed (which will
then need to trigger a thorough redisplay, I think).

Message #11 received at 74537 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 74537 <at> debbugs.gnu.org
Subject: Re: bug#74537: [PATCH] An on-disk image modification does a cache miss
Date: Tue, 26 Nov 2024 19:38:35 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Date: Mon, 25 Nov 2024 22:24:25 +0100
>> From:  Manuel Giraud via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>> 
>> With this patch, when an image file is modified on disk, the associated
>> cache image is invalidated and re-read from disk.
>
> Thanks, but I'm not sure it's a good idea to do that automatically
> whenever lookup_image is called.
>
> First, lookup_image is called only when we need to create or access an
> image with only its Lisp descriptor in hand; once the image has been
> displayed, that should happen relatively rarely.  For example, the
> glyph matrices used to manipulate the display record only the image's
> cached ID, and the code accesses the cached image without calling
> lookup_image.

Ok.

> So this change could have weird confusing effects, whereby the fact
> that the image was modified on disk becomes apparent only after some
> display-related actions, but not after others.  For example, scrolling
> a window by a small amount will not notice the change on disk, whereas
> significant changes, especially when the image goes off the window and
> back into it, will show the change.  Don't you see such issues if you
> install the change?

I've tried a bit with image-mode and also with something like:

(insert (propertize "f" 'display '(image :file "/tmp/foo.jpg" :type jpeg :width 100)))

and, yes, I can see the behavior you describe.  But I also can't really
see why it is a problem: the image has changed!  At one point it should
be reflected in Emacs, no?

> Second, if the image is already on display, and the file changes on
> disk, some layout calculations could see different dimensions than
> what is actually on display, which will cause subtle bugs.  For
> example, what if there's a 'display' property such as this:
>
>   '(space :width (0.5 (image PROPS)))
>
> and an image with those same PROPS is already shown on display?  The
> code which implements the above calls lookup_image, which will
> simulate a cache miss if the file has changed, and will then load the
> new file, which could return dimensions different from the image
> already on display.  So the width of the space glyph produced by the
> above will be different from that of the displayed image, and we could
> have misalignment until the next redisplay which redraws the image.

This I've tried with:

(insert (propertize "foo" 'display '(space :width (0.1 . (image :file "/tmp/foo.jpg" :type jpeg)))))

and also the re-alignment occurs at /some/ point.  But likewise, I fail
to see why this is a problem.

After all, maybe I'm a bit partial to image-mode with this patch.  I
think my idea was to eventually get rid of the systematic `image-flush'
call in "image-mode.el" to make it beneficiate from the image cache more
and still be able to display the correct image.

It seems I also lack some imagination about how users could use images
in Emacs.  For instance, with your example about alignment, I imagined
that it could be something that a user would do in its modeline (for
example) but with images that would never ever change.
-- 
Manuel Giraud

Message #14 received at 74537 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: 74537 <at> debbugs.gnu.org
Subject: Re: bug#74537: [PATCH] An on-disk image modification does a cache miss
Date: Wed, 27 Nov 2024 16:17:28 +0200

> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: 74537 <at> debbugs.gnu.org
> Date: Tue, 26 Nov 2024 19:38:35 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > So this change could have weird confusing effects, whereby the fact
> > that the image was modified on disk becomes apparent only after some
> > display-related actions, but not after others.  For example, scrolling
> > a window by a small amount will not notice the change on disk, whereas
> > significant changes, especially when the image goes off the window and
> > back into it, will show the change.  Don't you see such issues if you
> > install the change?
> 
> I've tried a bit with image-mode and also with something like:
> 
> (insert (propertize "f" 'display '(image :file "/tmp/foo.jpg" :type jpeg :width 100)))
> 
> and, yes, I can see the behavior you describe.  But I also can't really
> see why it is a problem: the image has changed!  At one point it should
> be reflected in Emacs, no?

First, that isn't always true.  Consider a file visited in some Emacs
buffer -- we don't revert it automatically when the visited file on
disk changes, do we?  We have commands and minor modes to do that, but
they are optional.  Why should images be different?

But yes, it would be good to have a mode and command(s) to update the
images when/if they were modified.  We just must be sure we update
_all_ of them, so if an image is displayed in several buffers/windows,
the update affects all of them.  Otherwise we will have weird bugs
like the one above.  And since the display engine has its own ideas
for when to perform a thorough redisplay and when flush the image
cache, these bugs will be hard to reproduce and debug.

My point is that, since the display engine goes out of its way to try
not to do stuff that isn't necessary, it will not "re-lookup_image" if
it has no reason to believe what's on display is outdated.  The image
cache is there to make redisplay of windows showing images faster.
What you want here is to disable some of these optimizations under
certain circumstances, but that must be done correctly, to avoid
incorrect/inaccurate/confusing display.

> This I've tried with:
> 
> (insert (propertize "foo" 'display '(space :width (0.1 . (image :file "/tmp/foo.jpg" :type jpeg)))))
> 
> and also the re-alignment occurs at /some/ point.  But likewise, I fail
> to see why this is a problem.

It is a problem because what the Lisp program which used such a
property wanted (alignment on display) will not happen, since the
image on display and the one you generate by calling lookup_image in
the above scenario might have different metrics.  What we must somehow
do in this case (assuming the user indeed wants the images to
immediately reflect their disk file changes) is to update the image on
display as well.  One way of doing that is to perform a complete
redisplay of each window which shows the image.  I'm not sure, but it
could be the only way, which means such updates will be somewhat
expensive if one has many windows with the image.  (One trivial case
where an image is displayed many times is the tool bar, when you have
many frames.  Another case could be the mode line.)

> After all, maybe I'm a bit partial to image-mode with this patch.  I
> think my idea was to eventually get rid of the systematic `image-flush'
> call in "image-mode.el" to make it beneficiate from the image cache more
> and still be able to display the correct image.

Sorry, I don't understand: the patch you posted effectively flushes
the cache more often, so how is it more beneficial to users of the
cache?

> It seems I also lack some imagination about how users could use images
> in Emacs.  For instance, with your example about alignment, I imagined
> that it could be something that a user would do in its modeline (for
> example) but with images that would never ever change.

It will change if we completely redisplay the window (after removing
the image from the cache, or otherwise rejecting the cached image).

Message #17 received at 74537 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 74537 <at> debbugs.gnu.org
Subject: Re: bug#74537: [PATCH] An on-disk image modification does a cache miss
Date: Tue, 10 Dec 2024 17:42:56 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
>> Cc: 74537 <at> debbugs.gnu.org
>> Date: Tue, 26 Nov 2024 19:38:35 +0100
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> > So this change could have weird confusing effects, whereby the fact
>> > that the image was modified on disk becomes apparent only after some
>> > display-related actions, but not after others.  For example, scrolling
>> > a window by a small amount will not notice the change on disk, whereas
>> > significant changes, especially when the image goes off the window and
>> > back into it, will show the change.  Don't you see such issues if you
>> > install the change?
>> 
>> I've tried a bit with image-mode and also with something like:
>> 
>> (insert (propertize "f" 'display '(image :file "/tmp/foo.jpg" :type jpeg :width 100)))
>> 
>> and, yes, I can see the behavior you describe.  But I also can't really
>> see why it is a problem: the image has changed!  At one point it should
>> be reflected in Emacs, no?
>
> First, that isn't always true.  Consider a file visited in some Emacs
> buffer -- we don't revert it automatically when the visited file on
> disk changes, do we?  We have commands and minor modes to do that, but
> they are optional.  Why should images be different?
>
> But yes, it would be good to have a mode and command(s) to update the
> images when/if they were modified.  We just must be sure we update
> _all_ of them, so if an image is displayed in several buffers/windows,
> the update affects all of them.  Otherwise we will have weird bugs
> like the one above.  And since the display engine has its own ideas
> for when to perform a thorough redisplay and when flush the image
> cache, these bugs will be hard to reproduce and debug.
>
> My point is that, since the display engine goes out of its way to try
> not to do stuff that isn't necessary, it will not "re-lookup_image" if
> it has no reason to believe what's on display is outdated.  The image
> cache is there to make redisplay of windows showing images faster.
> What you want here is to disable some of these optimizations under
> certain circumstances, but that must be done correctly, to avoid
> incorrect/inaccurate/confusing display.
>
>> This I've tried with:
>> 
>> (insert (propertize "foo" 'display '(space :width (0.1 . (image :file "/tmp/foo.jpg" :type jpeg)))))
>> 
>> and also the re-alignment occurs at /some/ point.  But likewise, I fail
>> to see why this is a problem.
>
> It is a problem because what the Lisp program which used such a
> property wanted (alignment on display) will not happen, since the
> image on display and the one you generate by calling lookup_image in
> the above scenario might have different metrics.  What we must somehow
> do in this case (assuming the user indeed wants the images to
> immediately reflect their disk file changes) is to update the image on
> display as well.  One way of doing that is to perform a complete
> redisplay of each window which shows the image.  I'm not sure, but it
> could be the only way, which means such updates will be somewhat
> expensive if one has many windows with the image.  (One trivial case
> where an image is displayed many times is the tool bar, when you have
> many frames.  Another case could be the mode line.)
>
>> After all, maybe I'm a bit partial to image-mode with this patch.  I
>> think my idea was to eventually get rid of the systematic `image-flush'
>> call in "image-mode.el" to make it beneficiate from the image cache more
>> and still be able to display the correct image.
>
> Sorry, I don't understand: the patch you posted effectively flushes
> the cache more often, so how is it more beneficial to users of the
> cache?

It flushes the cache iff the on-disk image has changed (which is, of
course, rare)... I think this all boils down to the fact that I don't
really understand the purpose of the `image-flush' call in image-mode.el
-- 
Manuel Giraud

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.