Redisplay slower in Emacs 28 than Emacs 27

Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

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


Following the "The Emacs master is much slower than the emacs-27 branch." 
thread, I tested Alan's benchmark on the master branch.  The platform I 
used is a 64-bit GNU/Linux computer, with GCC 10.2.

It turns out that the 10-15% slowdown of redisplay in Emacs 28, compared 
to Emacs 27, is due to a single character change, namely the "!" character 
removed in commit 165fd028.

I attach three graphs of the timings calculated on various revisions of 
master.  The benchmark ran ten times on each revision; Emacs was compiled 
with CFLAGS='-O2'.  "timing-1.png" shows that the slowdown happens between 
revision 68ff32a5 and 7445560d, "timing-2.png" narrows this to revisions 
4c5043c5 to d3c73fbf, and "timing-3.png" identifies revision 165fd028 as 
the cause of the problem.

[-- Attachment #2: Type: image/png, Size: 12471 bytes --]

[-- Attachment #3: Type: image/png, Size: 25502 bytes --]

[-- Attachment #4: Type: image/png, Size: 10661 bytes --]

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Gregory Heytings via "Emacs development discussions."
<emacs-devel@gnu.org> writes:

> Following the "The Emacs master is much slower than the emacs-27
> branch." thread, I tested Alan's benchmark on the master branch.  The
> platform I used is a 64-bit GNU/Linux computer, with GCC 10.2.
>
> It turns out that the 10-15% slowdown of redisplay in Emacs 28,
> compared to Emacs 27, is due to a single character change, namely the
> "!" character removed in commit 165fd028.

That sounds rather odd, because the code in question is this:

  for (img = c->buckets[i]; img; img = img->next)
    if (img->hash == hash
	&& equal_lists (img->spec, spec)

And the ! removed was in front of the equal_list (which was reversed
logically).

In comparison, the code looks like this in Emacs 27:

  for (img = c->buckets[i]; img; img = img->next)
    if (img->hash == hash
	&& !NILP (Fequal (img->spec, spec))


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>> Following the "The Emacs master is much slower than the emacs-27 
>> branch." thread, I tested Alan's benchmark on the master branch.  The 
>> platform I used is a 64-bit GNU/Linux computer, with GCC 10.2.
>>
>> It turns out that the 10-15% slowdown of redisplay in Emacs 28, 
>> compared to Emacs 27, is due to a single character change, namely the 
>> "!" character removed in commit 165fd028.
>
> That sounds rather odd, because the code in question is this:
>
>  for (img = c->buckets[i]; img; img = img->next)
>    if (img->hash == hash
> 	&& equal_lists (img->spec, spec)
>
> And the ! removed was in front of the equal_list (which was reversed 
> logically).
>
> In comparison, the code looks like this in Emacs 27:
>
>  for (img = c->buckets[i]; img; img = img->next)
>    if (img->hash == hash
> 	&& !NILP (Fequal (img->spec, spec))
>

I was surprised, too.  What I do know is that I observed this 10-15% 
slowdown consistently, on that particular revision, so it cannot be 
accidental.  As you see on the graphs, I tested about hundred different 
revisions, each of them ten times.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Gregory Heytings <ghe@sdf.org> writes:

> I was surprised, too.  What I do know is that I observed this 10-15%
> slowdown consistently, on that particular revision, so it cannot be
> accidental.  As you see on the graphs, I tested about hundred
> different revisions, each of them ten times.

I have no explanation for that -- but if you add the "!" back, you
disable the Emacs cache completely.  Try it, and load a large image with

emacs -Q some-megapixel-image.jpg

and Emacs will become unusable, because it'll re-decode the image for
each update.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Jean Louis]

* Gregory Heytings via "Emacs development discussions. <emacs-devel@gnu.org> [2020-12-07 17:55]:
> 
> Following the "The Emacs master is much slower than the emacs-27 branch."
> thread, I tested Alan's benchmark on the master branch.  The platform I used
> is a 64-bit GNU/Linux computer, with GCC 10.2.
> 
> It turns out that the 10-15% slowdown of redisplay in Emacs 28, compared to
> Emacs 27, is due to a single character change, namely the "!" character
> removed in commit 165fd028.
> 
> I attach three graphs of the timings calculated on various revisions of
> master.  The benchmark ran ten times on each revision; Emacs was compiled
> with CFLAGS='-O2'.  "timing-1.png" shows that the slowdown happens between
> revision 68ff32a5 and 7445560d, "timing-2.png" narrows this to revisions
> 4c5043c5 to d3c73fbf, and "timing-3.png" identifies revision 165fd028 as the
> cause of the problem.

Great work! You may also document how you did that and maybe other
people can help in debugging this way "what went wrong".

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>> I was surprised, too.  What I do know is that I observed this 10-15% 
>> slowdown consistently, on that particular revision, so it cannot be 
>> accidental.  As you see on the graphs, I tested about hundred different 
>> revisions, each of them ten times.
>
> I have no explanation for that -- but if you add the "!" back, you 
> disable the Emacs cache completely.  Try it, and load a large image with
>
> emacs -Q some-megapixel-image.jpg
>
> and Emacs will become unusable, because it'll re-decode the image for 
> each update.
>

I have no explanation either, and I did not write that the solution was to 
add the "!" back ;-)

What is also odd is that the benchmark does not load any image: it loads 
the "xdisp.c" file and scrolls down until EOF.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Gregory Heytings <ghe@sdf.org> writes:

> What is also odd is that the benchmark does not load any image: it
> loads the "xdisp.c" file and scrolls down until EOF.

Any images in the mode line or the toolbar, by any chance?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>> What is also odd is that the benchmark does not load any image: it 
>> loads the "xdisp.c" file and scrolls down until EOF.
>
> Any images in the mode line or the toolbar, by any chance?
>

No, with the tests were ran with emacs -Q.  I forgot to mention this, it 
was too obvious to me.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>>> What is also odd is that the benchmark does not load any image: it 
>>> loads the "xdisp.c" file and scrolls down until EOF.
>> 
>> Any images in the mode line or the toolbar, by any chance?
>> 
>
> No, with the tests were ran with emacs -Q.  I forgot to mention this, it 
> was too obvious to me.
>

"were run", sorry ;-)

Re: Redisplay slower in Emacs 28 than Emacs 27

[Óscar Fuentes]

Gregory Heytings via "Emacs development discussions."
<emacs-devel@gnu.org> writes:

> I have no explanation either, and I did not write that the solution
> was to add the "!" back ;-)
>
> What is also odd is that the benchmark does not load any image: it
> loads the "xdisp.c" file and scrolls down until EOF.

Did you `make bootstrap'? My experience bisecting Emacs says that
reliable results require full builds.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Gregory Heytings <ghe@sdf.org> writes:

> No, with the tests were ran with emacs -Q.  I forgot to mention this,
> it was too obvious to me.

But "emacs -Q" does show images in the tool bar.  :-) (Very small ones,
though.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>> I have no explanation either, and I did not write that the solution was 
>> to add the "!" back ;-)
>>
>> What is also odd is that the benchmark does not load any image: it 
>> loads the "xdisp.c" file and scrolls down until EOF.
>
> Did you `make bootstrap'? My experience bisecting Emacs says that 
> reliable results require full builds.
>

Yes, of course, each built was made on a freshly cloned copy.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> Date: Mon, 07 Dec 2020 15:28:18 +0000
> cc: emacs-devel@gnu.org
> From: Gregory Heytings via "Emacs development discussions." <emacs-devel@gnu.org>
> 
> What is also odd is that the benchmark does not load any image: it loads 
> the "xdisp.c" file and scrolls down until EOF.

It could be because of the images we show in the tool bar, or the
bitmaps on the fringes.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>> No, with the tests were ran with emacs -Q.  I forgot to mention this, 
>> it was too obvious to me.
>
> But "emacs -Q" does show images in the tool bar.  :-) (Very small ones, 
> though.)
>

Yes, and as you probably understood, by "does not load any image" I meant 
"in a buffer"... ;-)

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> Date: Mon, 07 Dec 2020 16:46:27 +0000
> cc: emacs-devel@gnu.org
> From: Gregory Heytings via "Emacs development discussions." <emacs-devel@gnu.org>
> 
> >> No, with the tests were ran with emacs -Q.  I forgot to mention this, 
> >> it was too obvious to me.
> >
> > But "emacs -Q" does show images in the tool bar.  :-) (Very small ones, 
> > though.)
> 
> Yes, and as you probably understood, by "does not load any image" I meant 
> "in a buffer"... ;-)

You asked why Emacs messes with the image cache although there are no
images in the buffer, and the answer is probably that Emacs doesn't
mess with images in the buffer, it messes with images elsewhere on
display.  Depending on whether your build uses GTK, this could happen
each time Emacs scrolls the window, because it then examines the tool
bar in order to decide whether it needs to be redrawn.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>>>> No, with the tests were ran with emacs -Q.  I forgot to mention this, 
>>>> it was too obvious to me.
>>>
>>> But "emacs -Q" does show images in the tool bar.  :-) (Very small 
>>> ones, though.)
>>
>> Yes, and as you probably understood, by "does not load any image" I 
>> meant "in a buffer"... ;-)
>
> You asked why Emacs messes with the image cache although there are no 
> images in the buffer,
>

No, I did not ask this.

>
> and the answer is probably that Emacs doesn't mess with images in the 
> buffer, it messes with images elsewhere on display.  Depending on 
> whether your build uses GTK, this could happen each time Emacs scrolls 
> the window, because it then examines the tool bar in order to decide 
> whether it needs to be redrawn.
>

I don't know, that's possible indeed.

In the other thread I referenced in my initial message, you wrote: "If 
someone can afford [to search the history to find the source of the 
slowdown], it would be nice to know.  Assuming a single commit causes it, 
that is (it could be several commits instead, each one slowing down Emacs 
by a few percents)."  I did exactly that, no more, no less.

That being said, it seems to me that this slowdown is avoidable.  Lars 
considers that the minor modification he made should not have changed 
anything.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Gregory Heytings <ghe@sdf.org> writes:

> That being said, it seems to me that this slowdown is avoidable.  Lars
> considers that the minor modification he made should not have changed
> anything.

Would it be possible for you to run the benchmarks on the current trunk,
with and without that "!"?  Because I just don't understand why
disabling the image cache would make Emacs slower.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>> That being said, it seems to me that this slowdown is avoidable.  Lars 
>> considers that the minor modification he made should not have changed 
>> anything.
>
> Would it be possible for you to run the benchmarks on the current trunk, 
> with and without that "!"?  Because I just don't understand why 
> disabling the image cache would make Emacs slower.
>

Sure, I'll do that.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Would it be possible for you to run the benchmarks on the current trunk,
> with and without that "!"?  Because I just don't understand why
> disabling the image cache would make Emacs slower.

I mean -- make Emacs faster.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Mon, 07 Dec 2020 19:47:35 +0100
> 
> Would it be possible for you to run the benchmarks on the current trunk,
> with and without that "!"?  Because I just don't understand why
> disabling the image cache would make Emacs slower.

Btw, there could be a way to speed up equal_lists, I think.  It
compares image specs, not just any 2 lists, right?  An image spec
always starts with the symbol 'image'.  So it should be enough to
compare only the list members starting from the second one.  The image
specs of the tool-bar buttons have 9 members, so this might yield an
11% speedup.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Alan Third]

On Mon, Dec 07, 2020 at 06:49:22PM +0000, Gregory Heytings via Emacs development discussions. wrote:
> 
> > > That being said, it seems to me that this slowdown is avoidable.
> > > Lars considers that the minor modification he made should not have
> > > changed anything.
> > 
> > Would it be possible for you to run the benchmarks on the current trunk,
> > with and without that "!"?  Because I just don't understand why
> > disabling the image cache would make Emacs slower.
> > 
> 
> Sure, I'll do that.

Could you also try with this patch:

@@ -2351,7 +2356,7 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
 
   /* Look up SPEC in the hash table of the image cache.  */
   hash = sxhash (spec);
-  img = search_image_cache (f, spec, hash, foreground, background, true);
+  img = search_image_cache (f, spec, hash, foreground, background, false);
   if (img && img->load_failed_p)
     {
       free_image (f, img);

It's a definite mistake on my part and may affect the caching of
images, although I can't see any way that it would slow anything down.
-- 
Alan Third

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>>> That being said, it seems to me that this slowdown is avoidable. 
>>> Lars considers that the minor modification he made should not have 
>>> changed anything.
>> 
>> Would it be possible for you to run the benchmarks on the current 
>> trunk, with and without that "!"?  Because I just don't understand why 
>> disabling the image cache would make Emacs slower.
>
> Sure, I'll do that.
>

I just did this on revision fc54c835 (Dec 5).

With equal_lists 18.7s, with !equal_lists 16.1s.

This difference is not due to an obscure compiler bug, the timings above 
are with GCC 10, but the timings with Clang 9 are similar: with 
equal_lists 19.0s, with !equal_lists 16.6s.

The difference vanishes when the toolbar is disabled: with equal_lists 
15.6s, with !equal_lists 15.7s.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Gregory Heytings <ghe@sdf.org> writes:

> With equal_lists 18.7s, with !equal_lists 16.1s.
>
> This difference is not due to an obscure compiler bug, the timings
> above are with GCC 10, but the timings with Clang 9 are similar: with
> equal_lists 19.0s, with !equal_lists 16.6s.

Very interesting.  Using the image cache for small images slows Emacs
down?  Or is there a bug in equal_lists?

Just to make 100% sure -- could you re-run the benchmark with something
like the following as the function:

static struct image *
search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
                    unsigned long foreground, unsigned long background,
                    bool ignore_colors)
{
  return NULL;
}


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Alan Third]

On Mon, Dec 07, 2020 at 10:15:43PM +0100, Lars Ingebrigtsen wrote:
> Gregory Heytings <ghe@sdf.org> writes:
> 
> > With equal_lists 18.7s, with !equal_lists 16.1s.
> >
> > This difference is not due to an obscure compiler bug, the timings
> > above are with GCC 10, but the timings with Clang 9 are similar: with
> > equal_lists 19.0s, with !equal_lists 16.6s.
> 
> Very interesting.  Using the image cache for small images slows Emacs
> down?  Or is there a bug in equal_lists?
> 
> Just to make 100% sure -- could you re-run the benchmark with something
> like the following as the function:
> 
> static struct image *
> search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
>                     unsigned long foreground, unsigned long background,
>                     bool ignore_colors)
> {
>   return NULL;
> }

I already tried that and it absolutely killed Emacs when the toolbar
was displayed.

As far as I can tell the "!" makes no great difference to the toolbar
images as it somehow finds a matching image in the cache anyway.

-- 
Alan Third

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>>>> That being said, it seems to me that this slowdown is avoidable. Lars 
>>>> considers that the minor modification he made should not have changed 
>>>> anything.
>>>
>>> Would it be possible for you to run the benchmarks on the current 
>>> trunk, with and without that "!"?  Because I just don't understand why 
>>> disabling the image cache would make Emacs slower.
>>
>> Sure, I'll do that.
>
> Could you also try with this patch:
>
> @@ -2351,7 +2356,7 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
>
>   /* Look up SPEC in the hash table of the image cache.  */
>   hash = sxhash (spec);
> -  img = search_image_cache (f, spec, hash, foreground, background, true);
> +  img = search_image_cache (f, spec, hash, foreground, background, false);
>   if (img && img->load_failed_p)
>     {
>       free_image (f, img);
>
> It's a definite mistake on my part and may affect the caching of images, 
> although I can't see any way that it would slow anything down.
>

That doesn't fix the problem: 18.7s without this patch, 18.6s with this 
patch.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> I already tried that and it absolutely killed Emacs when the toolbar
> was displayed.
>
> As far as I can tell the "!" makes no great difference to the toolbar
> images as it somehow finds a matching image in the cache anyway.

There's something very odd going on here.  I did the following:

  img = search_image_cache (f, spec, hash, foreground, background, true);
  if (img)
    printf("cached\n");
  else
    printf("uncached\n");

And then "emacs -Q", and switching between the *scratch* buffer and the
*Messages* buffer, I get:

uncached
uncached
uncached
uncached
uncached
uncached
uncached
[etc]

These are from the toolbar images.

However, when loading an image in image-mode, I get:

uncached
cached
cached
cached
cached
cached
cached

So there's something off about how the image specs are being compared in
the two cases.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>>> With equal_lists 18.7s, with !equal_lists 16.1s.
>>>
>>> This difference is not due to an obscure compiler bug, the timings 
>>> above are with GCC 10, but the timings with Clang 9 are similar: with 
>>> equal_lists 19.0s, with !equal_lists 16.6s.
>>
>> Very interesting.  Using the image cache for small images slows Emacs 
>> down?  Or is there a bug in equal_lists?
>>
>> Just to make 100% sure -- could you re-run the benchmark with something 
>> like the following as the function:
>>
>> static struct image *
>> search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
>>                     unsigned long foreground, unsigned long background,
>>                     bool ignore_colors)
>> {
>>   return NULL;
>> }
>
> I already tried that and it absolutely killed Emacs when the toolbar was 
> displayed.
>

Not on my computer.  I changed "if (!c) return NULL;" into "return NULL;" 
and it worked.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> So there's something off about how the image specs are being compared in
> the two cases.

With the following instrumentation:

diff --git a/src/image.c b/src/image.c
index 5eb4132295..f89649c61d 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1597,8 +1597,12 @@ make_image_cache (void)
 static bool
 equal_lists (Lisp_Object a, Lisp_Object b)
 {
+  CALLN (Fmessage, build_string("%S %S"),
+	 a, b);
   while (CONSP (a) && CONSP (b) && EQ (XCAR (a), XCAR (b)))
     a = XCDR (a), b = XCDR (b);
+  CALLN (Fmessage, build_string("%S %S"),
+	 a, b);
 
   return EQ (a, b);
 }

I consistently get this from the toolbar icons:

(image :type xpm :file "/home/larsi/src/emacs/trunk/etc/images/search.xpm") (image :type xpm :file "/home/larsi/src/emacs/trunk/etc/images/search.xpm")
("/home/larsi/src/emacs/trunk/etc/images/search.xpm") ("/home/larsi/src/emacs/trunk/etc/images/search.xpm")

That is, the two filenames are not EQ.  Is the toolbar code recreating
the filenames on every redisplay or something?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>> With equal_lists 18.7s, with !equal_lists 16.1s.
>>
>> This difference is not due to an obscure compiler bug, the timings 
>> above are with GCC 10, but the timings with Clang 9 are similar: with 
>> equal_lists 19.0s, with !equal_lists 16.6s.
>
> Very interesting.  Using the image cache for small images slows Emacs 
> down?  Or is there a bug in equal_lists?
>
> Just to make 100% sure -- could you re-run the benchmark with something 
> like the following as the function:
>
> static struct image *
> search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
>                    unsigned long foreground, unsigned long background,
>                    bool ignore_colors)
> {
>  return NULL;
> }
>

Sure.  As I just wrote to Alan, I changed "if (!c) return NULL;" into 
"return NULL;", and the result is: 18.6s, similar to the equal_lists case. 
As always, it's the average of ten runs.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

And this is the culprit -- i.e., this is what's busting the cache, as
far as I can tell:

(defun tool-bar--image-expression (icon)
  "Return an expression that evaluates to an image spec for ICON."
  (let* ((fg (face-attribute 'tool-bar :foreground))
	 (bg (face-attribute 'tool-bar :background))
	 (colors (nconc (if (eq fg 'unspecified) nil (list :foreground fg))
			(if (eq bg 'unspecified) nil (list :background bg))))
	 (xpm-spec (list :type 'xpm :file (concat icon ".xpm")))
	 (xpm-lo-spec (list :type 'xpm :file
			    (concat "low-color/" icon ".xpm")))
	 (pbm-spec (append (list :type 'pbm :file
                                 (concat icon ".pbm")) colors))
	 (xbm-spec (append (list :type 'xbm :file
                                 (concat icon ".xbm")) colors)))
    `(find-image (cond ((not (display-color-p))
			',(list pbm-spec xbm-spec xpm-lo-spec xpm-spec))
		       ((< (display-color-cells) 256)
			',(list xpm-lo-spec xpm-spec pbm-spec xbm-spec))
		       (t
			',(list xpm-spec pbm-spec xbm-spec))))))

Whenever Emacs is asked to redisplay the toolbar, Emacs will run this
function?  That's what my instrumentation seems to say...

I don't use toolbars normally, so I'm not sure how they're supposed to
work -- but shouldn't this toolbar data be computed only once, instead
of repeatedly?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> And this is the culprit -- i.e., this is what's busting the cache, as
> far as I can tell:

With this change, the toolbar should no longer bust the image cache.

Gregory, can you benchmark with this change (on an otherwise unchanged
trunk, i.e., with equal_lists, not !equal_lists)?

diff --git a/lisp/tool-bar.el b/lisp/tool-bar.el
index 8456216430..c3d0841d0c 100644
--- a/lisp/tool-bar.el
+++ b/lisp/tool-bar.el
@@ -154,12 +154,12 @@ tool-bar--image-expression
                                  (concat icon ".pbm")) colors))
 	 (xbm-spec (append (list :type 'xbm :file
                                  (concat icon ".xbm")) colors)))
-    `(find-image (cond ((not (display-color-p))
-			',(list pbm-spec xbm-spec xpm-lo-spec xpm-spec))
-		       ((< (display-color-cells) 256)
-			',(list xpm-lo-spec xpm-spec pbm-spec xbm-spec))
-		       (t
-			',(list xpm-spec pbm-spec xbm-spec))))))
+    `(cond ((not (display-color-p))
+	    ',(find-image (list pbm-spec xbm-spec xpm-lo-spec xpm-spec)))
+	   ((< (display-color-cells) 256)
+	    ',(find-image (list xpm-lo-spec xpm-spec pbm-spec xbm-spec)))
+	   (t
+	    ',(find-image (list xpm-spec pbm-spec xbm-spec))))))
 
 ;;;###autoload
 (defun tool-bar-local-item (icon def key map &rest props)


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Alan Third]

On Mon, Dec 07, 2020 at 10:59:24PM +0100, Lars Ingebrigtsen wrote:
> And this is the culprit -- i.e., this is what's busting the cache, as
> far as I can tell:
> 
> (defun tool-bar--image-expression (icon)
>   "Return an expression that evaluates to an image spec for ICON."
>   (let* ((fg (face-attribute 'tool-bar :foreground))
> 	 (bg (face-attribute 'tool-bar :background))
> 	 (colors (nconc (if (eq fg 'unspecified) nil (list :foreground fg))
> 			(if (eq bg 'unspecified) nil (list :background bg))))
> 	 (xpm-spec (list :type 'xpm :file (concat icon ".xpm")))
> 	 (xpm-lo-spec (list :type 'xpm :file
> 			    (concat "low-color/" icon ".xpm")))
> 	 (pbm-spec (append (list :type 'pbm :file
>                                  (concat icon ".pbm")) colors))
> 	 (xbm-spec (append (list :type 'xbm :file
>                                  (concat icon ".xbm")) colors)))
>     `(find-image (cond ((not (display-color-p))
> 			',(list pbm-spec xbm-spec xpm-lo-spec xpm-spec))
> 		       ((< (display-color-cells) 256)
> 			',(list xpm-lo-spec xpm-spec pbm-spec xbm-spec))
> 		       (t
> 			',(list xpm-spec pbm-spec xbm-spec))))))
> 
> Whenever Emacs is asked to redisplay the toolbar, Emacs will run this
> function?  That's what my instrumentation seems to say...
> 
> I don't use toolbars normally, so I'm not sure how they're supposed to
> work -- but shouldn't this toolbar data be computed only once, instead
> of repeatedly?

I think the image cache is actually broken.

If we have two image specs that have the same contents, but don't have
identical memory locations, surely we still want the cache to work?

For example when playing an animated gif image.el doesn't store the
image specs for each frame, it generates them on the fly by
incrementing the frame counter. I can see that Emacs creates a new
cache entry for every frame every time the gif is played through.

That means it must be decoding every frame every time.

-- 
Alan Third

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> I think the image cache is actually broken.
>
> If we have two image specs that have the same contents, but don't have
> identical memory locations, surely we still want the cache to work?

We used to use Fequal on the spec, so that used to work.  I thought the
point of the equal_lists function was to speed that up a bit, by
dropping that generality and just using EQ instead?  But it does indeed
mean that you have to be more careful with how you construct the image
specs, and I think that's a fair tradeoff.

It works for the vast majority of the use cases, but as we saw here, the
toolbar code hadn't been adjusted, but that's easily fixed (see the
patch I posted earlier).

Especially with huge :data images, the equal_lists code should be faster
than the Fequal code, I think?  On the other hand, when we're
constructing the hashes, we iterate over a lot of data anyway, so...

> For example when playing an animated gif image.el doesn't store the
> image specs for each frame, it generates them on the fly by
> incrementing the frame counter. I can see that Emacs creates a new
> cache entry for every frame every time the gif is played through.
>
> That means it must be decoding every frame every time.

Yes, I think so.  I think there's at least one open bug report about
making animated gifs faster (because they're really slow now).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

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


> With this change, the toolbar should no longer bust the image cache.
>
> Gregory, can you benchmark with this change (on an otherwise unchanged 
> trunk, i.e., with equal_lists, not !equal_lists)?
>
> diff --git a/lisp/tool-bar.el b/lisp/tool-bar.el
> index 8456216430..c3d0841d0c 100644
> --- a/lisp/tool-bar.el
> +++ b/lisp/tool-bar.el
> @@ -154,12 +154,12 @@ tool-bar--image-expression
>                                  (concat icon ".pbm")) colors))
> 	 (xbm-spec (append (list :type 'xbm :file
>                                  (concat icon ".xbm")) colors)))
> -    `(find-image (cond ((not (display-color-p))
> -			',(list pbm-spec xbm-spec xpm-lo-spec xpm-spec))
> -		       ((< (display-color-cells) 256)
> -			',(list xpm-lo-spec xpm-spec pbm-spec xbm-spec))
> -		       (t
> -			',(list xpm-spec pbm-spec xbm-spec))))))
> +    `(cond ((not (display-color-p))
> +	    ',(find-image (list pbm-spec xbm-spec xpm-lo-spec xpm-spec)))
> +	   ((< (display-color-cells) 256)
> +	    ',(find-image (list xpm-lo-spec xpm-spec pbm-spec xbm-spec)))
> +	   (t
> +	    ',(find-image (list xpm-spec pbm-spec xbm-spec))))))
>
> ;;;###autoload
> (defun tool-bar-local-item (icon def key map &rest props)
>

Hmmm... this fails with:

Loading /home/ghe/local/src/emacs/lisp/indent.el (source)...
Loading /home/ghe/local/src/emacs/lisp/emacs-lisp/cl-generic.el (source)...
Eager macro-expansion failure: (void-variable image-load-path)
Eager macro-expansion failure: (void-variable image-load-path)
Eager macro-expansion failure: (void-variable image-load-path)
Eager macro-expansion failure: (void-variable image-load-path)
Eager macro-expansion failure: (void-variable image-load-path)
Eager macro-expansion failure: (void-variable image-load-path)
Eager macro-expansion failure: (void-variable image-load-path)
Eager macro-expansion failure: (void-variable image-load-path)
Symbol’s value as variable is void: image-load-path

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Gregory Heytings <ghe@sdf.org> writes:

> Hmmm... this fails with:
>
> Loading /home/ghe/local/src/emacs/lisp/indent.el (source)...
> Loading /home/ghe/local/src/emacs/lisp/emacs-lisp/cl-generic.el (source)...
> Eager macro-expansion failure: (void-variable image-load-path)
> Eager macro-expansion failure: (void-variable image-load-path)
> Eager macro-expansion failure: (void-variable image-load-path)
> Eager macro-expansion failure: (void-variable image-load-path)
> Eager macro-expansion failure: (void-variable image-load-path)
> Eager macro-expansion failure: (void-variable image-load-path)
> Eager macro-expansion failure: (void-variable image-load-path)
> Eager macro-expansion failure: (void-variable image-load-path)
> Symbol’s value as variable is void: image-load-path

Hm.  Does adding a (require 'image) help?  Like this:

diff --git a/lisp/tool-bar.el b/lisp/tool-bar.el
index 8456216430..2d929214d5 100644
--- a/lisp/tool-bar.el
+++ b/lisp/tool-bar.el
@@ -141,6 +141,8 @@ tool-bar-add-item
 To define items in any other map, use `tool-bar-local-item'."
   (apply #'tool-bar-local-item icon def key tool-bar-map props))
 
+(require 'image)
+
 (defun tool-bar--image-expression (icon)
   "Return an expression that evaluates to an image spec for ICON."
   (let* ((fg (face-attribute 'tool-bar :foreground))
@@ -154,12 +156,12 @@ tool-bar--image-expression
                                  (concat icon ".pbm")) colors))
 	 (xbm-spec (append (list :type 'xbm :file
                                  (concat icon ".xbm")) colors)))
-    `(find-image (cond ((not (display-color-p))
-			',(list pbm-spec xbm-spec xpm-lo-spec xpm-spec))
-		       ((< (display-color-cells) 256)
-			',(list xpm-lo-spec xpm-spec pbm-spec xbm-spec))
-		       (t
-			',(list xpm-spec pbm-spec xbm-spec))))))
+    `(cond ((not (display-color-p))
+	    ',(find-image (list pbm-spec xbm-spec xpm-lo-spec xpm-spec)))
+	   ((< (display-color-cells) 256)
+	    ',(find-image (list xpm-lo-spec xpm-spec pbm-spec xbm-spec)))
+	   (t
+	    ',(find-image (list xpm-spec pbm-spec xbm-spec))))))
 
 ;;;###autoload
 (defun tool-bar-local-item (icon def key map &rest props)


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>
> Hm.  Does adding a (require 'image) help?  Like this:
>

No it doesn't :-(

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Gregory Heytings <ghe@sdf.org> writes:

>>
>> Hm.  Does adding a (require 'image) help?  Like this:
>>
>
> No it doesn't :-(

No, that one wasn't any good -- I didn't understand why it failed for
you and not for me, but it's a bootstrap problem.

This one should work, I think.  (Use instead of the other patch, not in
addition to.)

diff --git a/lisp/image.el b/lisp/image.el
index 9ebb603086..ee961ce577 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -679,8 +679,10 @@ image-search-load-path
       (setq path (cdr path)))
     (if found filename)))
 
+(defvar find-image--cache (make-hash-table :test #'equal))
+
 ;;;###autoload
-(defun find-image (specs)
+(defun find-image (specs &optional cache)
   "Find an image, choosing one of a list of image specifications.
 
 SPECS is a list of image specifications.
@@ -698,23 +700,29 @@ find-image
 The image is looked for in `image-load-path'.
 
 Image files should not be larger than specified by `max-image-size'."
-  (let (image)
-    (while (and specs (null image))
-      (let* ((spec (car specs))
-	     (type (plist-get spec :type))
-	     (data (plist-get spec :data))
-	     (file (plist-get spec :file))
-	     found)
-	(when (image-type-available-p type)
-	  (cond ((stringp file)
-		 (if (setq found (image-search-load-path file))
-		     (setq image
-			   (cons 'image (plist-put (copy-sequence spec)
-						   :file found)))))
-		((not (null data))
-		 (setq image (cons 'image spec)))))
-	(setq specs (cdr specs))))
-    image))
+  (or (and cache
+           (gethash specs find-image--cache))
+      (let ((orig-specs specs)
+            image)
+        (message "find: %S" specs)
+        (while (and specs (null image))
+          (let* ((spec (car specs))
+	         (type (plist-get spec :type))
+	         (data (plist-get spec :data))
+	         (file (plist-get spec :file))
+	         found)
+	    (when (image-type-available-p type)
+	      (cond ((stringp file)
+		     (if (setq found (image-search-load-path file))
+		         (setq image
+			       (cons 'image (plist-put (copy-sequence spec)
+						       :file found)))))
+		    ((not (null data))
+		     (setq image (cons 'image spec)))))
+	    (setq specs (cdr specs))))
+        (when cache
+          (setf (gethash orig-specs find-image--cache) image))
+        image)))
 
 
 ;;;###autoload


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Gah, I left some debugging in the previous patch.  This one, then:

diff --git a/lisp/image.el b/lisp/image.el
index 9ebb603086..4b6710e4a3 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -679,8 +679,10 @@ image-search-load-path
       (setq path (cdr path)))
     (if found filename)))
 
+(defvar find-image--cache (make-hash-table :test #'equal))
+
 ;;;###autoload
-(defun find-image (specs)
+(defun find-image (specs &optional cache)
   "Find an image, choosing one of a list of image specifications.
 
 SPECS is a list of image specifications.
@@ -698,23 +700,28 @@ find-image
 The image is looked for in `image-load-path'.
 
 Image files should not be larger than specified by `max-image-size'."
-  (let (image)
-    (while (and specs (null image))
-      (let* ((spec (car specs))
-	     (type (plist-get spec :type))
-	     (data (plist-get spec :data))
-	     (file (plist-get spec :file))
-	     found)
-	(when (image-type-available-p type)
-	  (cond ((stringp file)
-		 (if (setq found (image-search-load-path file))
-		     (setq image
-			   (cons 'image (plist-put (copy-sequence spec)
-						   :file found)))))
-		((not (null data))
-		 (setq image (cons 'image spec)))))
-	(setq specs (cdr specs))))
-    image))
+  (or (and cache
+           (gethash specs find-image--cache))
+      (let ((orig-specs specs)
+            image)
+        (while (and specs (null image))
+          (let* ((spec (car specs))
+	         (type (plist-get spec :type))
+	         (data (plist-get spec :data))
+	         (file (plist-get spec :file))
+	         found)
+	    (when (image-type-available-p type)
+	      (cond ((stringp file)
+		     (if (setq found (image-search-load-path file))
+		         (setq image
+			       (cons 'image (plist-put (copy-sequence spec)
+						       :file found)))))
+		    ((not (null data))
+		     (setq image (cons 'image spec)))))
+	    (setq specs (cdr specs))))
+        (when cache
+          (setf (gethash orig-specs find-image--cache) image))
+        image)))
 
 
 ;;;###autoload


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

It's getting a bit late, I think.  This is the one!  For sure:


diff --git a/lisp/image.el b/lisp/image.el
index 9ebb603086..4b6710e4a3 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -679,8 +679,10 @@ image-search-load-path
       (setq path (cdr path)))
     (if found filename)))
 
+(defvar find-image--cache (make-hash-table :test #'equal))
+
 ;;;###autoload
-(defun find-image (specs)
+(defun find-image (specs &optional cache)
   "Find an image, choosing one of a list of image specifications.
 
 SPECS is a list of image specifications.
@@ -698,23 +700,28 @@ find-image
 The image is looked for in `image-load-path'.
 
 Image files should not be larger than specified by `max-image-size'."
-  (let (image)
-    (while (and specs (null image))
-      (let* ((spec (car specs))
-	     (type (plist-get spec :type))
-	     (data (plist-get spec :data))
-	     (file (plist-get spec :file))
-	     found)
-	(when (image-type-available-p type)
-	  (cond ((stringp file)
-		 (if (setq found (image-search-load-path file))
-		     (setq image
-			   (cons 'image (plist-put (copy-sequence spec)
-						   :file found)))))
-		((not (null data))
-		 (setq image (cons 'image spec)))))
-	(setq specs (cdr specs))))
-    image))
+  (or (and cache
+           (gethash specs find-image--cache))
+      (let ((orig-specs specs)
+            image)
+        (while (and specs (null image))
+          (let* ((spec (car specs))
+	         (type (plist-get spec :type))
+	         (data (plist-get spec :data))
+	         (file (plist-get spec :file))
+	         found)
+	    (when (image-type-available-p type)
+	      (cond ((stringp file)
+		     (if (setq found (image-search-load-path file))
+		         (setq image
+			       (cons 'image (plist-put (copy-sequence spec)
+						       :file found)))))
+		    ((not (null data))
+		     (setq image (cons 'image spec)))))
+	    (setq specs (cdr specs))))
+        (when cache
+          (setf (gethash orig-specs find-image--cache) image))
+        image)))
 
 
 ;;;###autoload
diff --git a/lisp/tool-bar.el b/lisp/tool-bar.el
index 8456216430..37f42be3f4 100644
--- a/lisp/tool-bar.el
+++ b/lisp/tool-bar.el
@@ -159,7 +159,8 @@ tool-bar--image-expression
 		       ((< (display-color-cells) 256)
 			',(list xpm-lo-spec xpm-spec pbm-spec xbm-spec))
 		       (t
-			',(list xpm-spec pbm-spec xbm-spec))))))
+			',(list xpm-spec pbm-spec xbm-spec)))
+                 t)))
 
 ;;;###autoload
 (defun tool-bar-local-item (icon def key map &rest props)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>
> Gah, I left some debugging in the previous patch.  This one, then:
>

This one has no effect: 18.6s.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Gregory Heytings via Emacs development discussions.]

>
> It's getting a bit late, I think.  This is the one!  For sure:
>

And this one works: 16.3s!  Congratulations!  :-)

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Gregory Heytings <ghe@sdf.org> writes:

>> It's getting a bit late, I think.  This is the one!  For sure:
>
> And this one works: 16.3s!  Congratulations!  :-)

*phew*

Thanks for all the benchmarking.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Anyway, this has reminded me of something I've wondered about before:
Why doesn't `create-image' stick a :hash element into the spec so that
redisplay doesn't have to recompute it all the time?  Especially with
large datap images, not having to recompute the hash when redisplaying
must surely be a big win...

The wrinkle is that you'd have to recompute it if you alter the image
spec, but I think that should be OK.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Anyway, this has reminded me of something I've wondered about before:
> Why doesn't `create-image' stick a :hash element into the spec so that
> redisplay doesn't have to recompute it all the time?  Especially with
> large datap images, not having to recompute the hash when redisplaying
> must surely be a big win...

On the other hand, since we're doing the comparison with EQ, it makes
little sense to compute the hash with a function that takes string
contents into consideration.  So we would make a much simpler and faster
hash function for the image cache that just looks at object identity
(and number values) and not anything else.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Btw, there could be a way to speed up equal_lists, I think.  It
> compares image specs, not just any 2 lists, right?  An image spec
> always starts with the symbol 'image'.  So it should be enough to
> compare only the list members starting from the second one.  The image
> specs of the tool-bar buttons have 9 members, so this might yield an
> 11% speedup.

Yup.  And the same can be done when computing the image hash -- which
should be special-purpose and use identity and not contents.

But before doing that, we should probably decide, once and for all,
whether the cache should be on spec equality or identity...

And if we really go with identity (which we do now, but...  wrognly), we
could go one step further, and just look at the identity of the spec
itself, instead of looking at the identity of the contents.

That is, use

	&& EQ (img->spec, spec)

instead of

	&& equal_lists (img->spec, spec)

Or, if we decide to revert to a more permissive cache again, we could
just go back to using

	&& !NILP (Fequal (img->spec, spec))

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Tue, 08 Dec 2020 00:48:24 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> +  (or (and cache
> +           (gethash specs find-image--cache))
> +      (let ((orig-specs specs)
> +            image)
> +        (while (and specs (null image))

What will this do when the image file on disk changes?  AFAIU, the
current implementation will use the new image for the tool bar on the
next redisplay opportunity, but this change will make us continue
using the cached one?

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> What will this do when the image file on disk changes?  AFAIU, the
> current implementation will use the new image for the tool bar on the
> next redisplay opportunity, but this change will make us continue
> using the cached one?

Yes.  Changing toolbar image file contents is such a rare action that
it's not worth making Emacs recheck them on redisplay.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> What will this do when the image file on disk changes?  AFAIU, the
>> current implementation will use the new image for the tool bar on the
>> next redisplay opportunity, but this change will make us continue
>> using the cached one?
>
> Yes.  Changing toolbar image file contents is such a rare action that
> it's not worth making Emacs recheck them on redisplay.

Sorry, thinko -- what this new cache does is cache the image locations,
not the contents.  So if you add a new search.xpm somewhere in the load
path, the toolbar will continue using the previously found location.

If you change the image on file, there's no change from how this worked
in Emacs 27 -- Emacs will not show the new image, because it never
checks whether an image file is newer than what's in the image cache.

So before as now, you have to clear the image cache in that case.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: ghe@sdf.org,  emacs-devel@gnu.org
> Date: Tue, 08 Dec 2020 15:06:44 +0100
> 
> But before doing that, we should probably decide, once and for all,
> whether the cache should be on spec equality or identity...
> 
> And if we really go with identity (which we do now, but...  wrognly), we
> could go one step further, and just look at the identity of the spec
> itself, instead of looking at the identity of the contents.
> 
> That is, use
> 
> 	&& EQ (img->spec, spec)
> 
> instead of
> 
> 	&& equal_lists (img->spec, spec)

I think using EQ might break something, given that we've been using
Fequal for a long time.

> Or, if we decide to revert to a more permissive cache again, we could
> just go back to using
> 
> 	&& !NILP (Fequal (img->spec, spec))

We could do that, yeah.  But most of the members of an image spec are
symbols and suchlikes, so maybe we could use EQ for almost all of
them, except for strings?

Re: Redisplay slower in Emacs 28 than Emacs 27

[Stefan Monnier]

> We could do that, yeah.  But most of the members of an image spec are
> symbols and suchlikes, so maybe we could use EQ for almost all of
> them, except for strings?

But that's already what `Fequal` does, no?


        Stefan

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: ghe@sdf.org,  emacs-devel@gnu.org
> Date: Tue, 08 Dec 2020 16:07:29 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > What will this do when the image file on disk changes?  AFAIU, the
> > current implementation will use the new image for the tool bar on the
> > next redisplay opportunity, but this change will make us continue
> > using the cached one?
> 
> Yes.  Changing toolbar image file contents is such a rare action that
> it's not worth making Emacs recheck them on redisplay.

All we need to do is stat the file, and reload it if its time stamp or
size changed.  Why is that a problem?  If we don't do that, we are
making an incompatible change, and I don't see a reason for doing that
unconditionally.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: ghe@sdf.org,  emacs-devel@gnu.org
> Date: Tue, 08 Dec 2020 16:19:39 +0100
> 
> > Yes.  Changing toolbar image file contents is such a rare action that
> > it's not worth making Emacs recheck them on redisplay.
> 
> Sorry, thinko -- what this new cache does is cache the image locations,
> not the contents.  So if you add a new search.xpm somewhere in the load
> path, the toolbar will continue using the previously found location.
> 
> If you change the image on file, there's no change from how this worked
> in Emacs 27 -- Emacs will not show the new image, because it never
> checks whether an image file is newer than what's in the image cache.
> 
> So before as now, you have to clear the image cache in that case.

But before one could put a new image file in a different place on
image-load-path, and it would be used automatically instead of the old
one?  So if now this will not happen automatically, we should at least
document that and provide a function to do that manually.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  ghe@sdf.org,  emacs-devel@gnu.org
> Date: Tue, 08 Dec 2020 10:56:48 -0500
> 
> > We could do that, yeah.  But most of the members of an image spec are
> > symbols and suchlikes, so maybe we could use EQ for almost all of
> > them, except for strings?
> 
> But that's already what `Fequal` does, no?

That, and much more.  But maybe it doesn't matter?

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> That is, use
>> 
>> 	&& EQ (img->spec, spec)
>> 
>> instead of
>> 
>> 	&& equal_lists (img->spec, spec)
>
> I think using EQ might break something, given that we've been using
> Fequal for a long time.

Yeah, the equal_lists thing did break the toolbar cache, so there might
be other breakages.  On the other hand, it seems like it didn't break
much else, so we might want to continue in this direction to make
redisplay of images faster.

And moving to just EQ is one more step in that direction.

>> Or, if we decide to revert to a more permissive cache again, we could
>> just go back to using
>> 
>> 	&& !NILP (Fequal (img->spec, spec))
>
> We could do that, yeah.  But most of the members of an image spec are
> symbols and suchlikes, so maybe we could use EQ for almost all of
> them, except for strings?

Fequal is already pretty optimised, so in practice, it isn't that
different, really.  Its uses EQ for symbols and stuff, and I think it
compares strings with EQ first, and only looks at the contents of the
strings if they aren't EQ?  Or do I misremember?

So moving to Fequal is fine if we decide to have the Emacs 27 caching
semantics.

However, if we move to EQ semantics, the real speedup is that we don't
have to call sxhash(img) on all the images -- and that's a slow
operation, because it'll call hash_string on all the string contents,
and if you have a large :data image, it'll compute that every time Emacs
decides to redisplay that image.

My proposal is: Be explicit in documenting how images are cached, and
only cache images based on EQ-ness of the spec.  That will speed up
redisplay of large :data images substantially (since we never need to do
an sxhash on the spec).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> But before one could put a new image file in a different place on
> image-load-path, and it would be used automatically instead of the old
> one?

Yes.  (For the toolbars; the other lookups aren't cached.)

> So if now this will not happen automatically, we should at least
> document that and provide a function to do that manually.

It only seems like something a developer would encounter, I think?
(Negative lookups are not cached, so if you install a new package with
new toolbar icons, they'll be picked up.)

So neither seemed necessary to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: ghe@sdf.org,  emacs-devel@gnu.org
> Date: Tue, 08 Dec 2020 17:31:16 +0100
> 
> So moving to Fequal is fine if we decide to have the Emacs 27 caching
> semantics.
> 
> However, if we move to EQ semantics, the real speedup is that we don't
> have to call sxhash(img) on all the images -- and that's a slow
> operation, because it'll call hash_string on all the string contents,
> and if you have a large :data image, it'll compute that every time Emacs
> decides to redisplay that image.

I don't think I follow: since an image spec can (and usually does)
include a file name, how can we use EQ?  That would mean identical
file names will cause us to think an image isn't cached, right?  Or
what am I missing?

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: ghe@sdf.org,  emacs-devel@gnu.org
> Date: Tue, 08 Dec 2020 17:34:37 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > But before one could put a new image file in a different place on
> > image-load-path, and it would be used automatically instead of the old
> > one?
> 
> Yes.  (For the toolbars; the other lookups aren't cached.)
> 
> > So if now this will not happen automatically, we should at least
> > document that and provide a function to do that manually.
> 
> It only seems like something a developer would encounter, I think?

We don't know that.  For all I know there could be some 3rd party
package there playing fancy games with the tool-bar button that relies
on the current behavior.  After all, we've been behaving like that
since Emacs 21.

If no one needs that function, it will never be used.  But I think the
function should be quite simple, so adding it is a net win.  If we
document it, it will avoid a potential outcry.

Re: Redisplay slower in Emacs 28 than Emacs 27

[João Távora]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Gregory Heytings <ghe@sdf.org> writes:
>
>>> It's getting a bit late, I think.  This is the one!  For sure:
>>
>> And this one works: 16.3s!  Congratulations!  :-)
>
> *phew*
>
> Thanks for all the benchmarking.

What a gripping thread!  Who said emacs-devel isn't wholesome,
exciting, fun?  Congrats Gregory and Lars!

João

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> So moving to Fequal is fine if we decide to have the Emacs 27 caching
>> semantics.
>> 
>> However, if we move to EQ semantics, the real speedup is that we don't
>> have to call sxhash(img) on all the images -- and that's a slow
>> operation, because it'll call hash_string on all the string contents,
>> and if you have a large :data image, it'll compute that every time Emacs
>> decides to redisplay that image.
>
> I don't think I follow: since an image spec can (and usually does)
> include a file name, how can we use EQ?  That would mean identical
> file names will cause us to think an image isn't cached, right?  Or
> what am I missing?

I'm not sure -- I don't quite understand what you mean here.

Here's a spec:

(setq spec '(image :type "xpm" :file "/tmp/foo.xpm"))

Every time spec `spec' is redisplayed, EQ (spec, spec) will be true.
We'd be going by "object identity" of the spec, and not looking at the
contents at all.

I.e., we'd start treating image specs as (immutable) objects (to make
redisplay more efficient).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: ghe@sdf.org,  emacs-devel@gnu.org
> Date: Tue, 08 Dec 2020 18:29:24 +0100
> 
> > I don't think I follow: since an image spec can (and usually does)
> > include a file name, how can we use EQ?  That would mean identical
> > file names will cause us to think an image isn't cached, right?  Or
> > what am I missing?
> 
> I'm not sure -- I don't quite understand what you mean here.
> 
> Here's a spec:
> 
> (setq spec '(image :type "xpm" :file "/tmp/foo.xpm"))
> 
> Every time spec `spec' is redisplayed, EQ (spec, spec) will be true.

Not if the list is re-assembled again, right?

> We'd be going by "object identity" of the spec, and not looking at the
> contents at all.
> 
> I.e., we'd start treating image specs as (immutable) objects (to make
> redisplay more efficient).

It may make redisplay faster, but it will produce confusing effects if
an identical spec is constructed from the same or identical elements:
Emacs will think the image is not in the cache.  Sounds like a lot to
pay for a minor speedup, to say nothing of backward compatibility.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> It may make redisplay faster, but it will produce confusing effects if
> an identical spec is constructed from the same or identical elements:
> Emacs will think the image is not in the cache.  Sounds like a lot to
> pay for a minor speedup, to say nothing of backward compatibility.

It's a major speedup for :data images, which is what you'll find in eww
and mail readers.

And we've been using equal_lists in Emacs 28, which is incompatible in
much the same way, and we've seen only one thing that needed tweaking
because of that.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> We don't know that.  For all I know there could be some 3rd party
> package there playing fancy games with the tool-bar button that relies
> on the current behavior.  After all, we've been behaving like that
> since Emacs 21.
>
> If no one needs that function, it will never be used.  But I think the
> function should be quite simple, so adding it is a net win.  If we
> document it, it will avoid a potential outcry.

I find it astonishingly unlikely that somebody has written code that
puts identically-named images, for use in the toolbar, in different
directories, and then keeps moving those files around.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: ghe@sdf.org,  emacs-devel@gnu.org
> Date: Tue, 08 Dec 2020 18:51:18 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It may make redisplay faster, but it will produce confusing effects if
> > an identical spec is constructed from the same or identical elements:
> > Emacs will think the image is not in the cache.  Sounds like a lot to
> > pay for a minor speedup, to say nothing of backward compatibility.
> 
> It's a major speedup for :data images, which is what you'll find in eww
> and mail readers.
> 
> And we've been using equal_lists in Emacs 28, which is incompatible in
> much the same way, and we've seen only one thing that needed tweaking
> because of that.

Emacs 28 is still quite young.

Bottom line: I don't like such a change.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Stefan Monnier]

> Emacs 28 is still quite young.
> Bottom line: I don't like such a change.

I also think we should better understand why `Fequal` was a bad idea
before we can decide what's the best option.
Normally, `Fequal` should return super quickly if the two args are EQ.


        Stefan

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

I'm doing some benchmarking here, and my test case is to insert 200
big-ish images in a buffer, and then scrolling through the buffer.

:data image, current sxhash, equal_lists: 6.3s.
:data image, NOOP sxhash, equal_lists: 3.4s.

:file image, current sxhash, equal_lists: 3.4s.
:file image, NOOP sxhash, equal_lists: 3.4s.

:data image, current sxhash, Fequal: 6.4s.
:data image, NOOP sxhash, Fequal: 3.4s.

So -- the Fequal/equal_list doesn't make any difference.  As suspected,
the sxhash is the major opportunity to speed things up in this area.

Now, if we want to retain Fequal-ity (which is what Emacs 27 does), we
can't make sxhash more efficient here.

However, if we continue using equal_lists (which compares list members
with EQ), we can write a new hash function for image.c that just uses
identity/number values of the elements, and we'd get a nice speed-up.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> However, if we continue using equal_lists (which compares list members
> with EQ), we can write a new hash function for image.c that just uses
> identity/number values of the elements, and we'd get a nice speed-up.

Here's a stab as a new EQ-ey image spec hashing function:

diff --git a/src/fns.c b/src/fns.c
index e4c9acc316..2512bce5f5 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -4640,9 +4640,48 @@ sxhash_bignum (Lisp_Object bignum)
 }
 
 
+EMACS_UINT
+img_hash (Lisp_Object spec)
+{
+  EMACS_UINT hash = 0;
+
+  while (CONSP (spec))
+    {
+      EMACS_UINT val;
+      Lisp_Object obj = XCAR (spec);
+      spec = XCDR (spec);
+
+      switch (XTYPE (obj))
+	{
+	case_Lisp_Int:
+	  val = XUFIXNUM (obj);
+	  break;
+
+	case Lisp_Symbol:
+	  val = XHASH (obj);
+	  break;
+
+	case Lisp_String:
+	  val = (EMACS_UINT) SSDATA (obj);
+	  break;
+
+	case Lisp_Float:
+	  val = sxhash_float (XFLOAT_DATA (obj));
+	  break;
+
+	default:
+	  val = 0;
+	}
+
+      hash = sxhash_combine (hash, val);
+    }
+  return SXHASH_REDUCE (hash);
+}
+
 /* Return a hash code for OBJ.  DEPTH is the current depth in the Lisp
    structure.  Value is an unsigned integer clipped to INTMASK.  */
 
+
 EMACS_UINT
 sxhash (Lisp_Object obj)
 {
diff --git a/src/image.c b/src/image.c
index 5eb4132295..4c586853d8 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1634,6 +1634,7 @@ search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
 	&& (ignore_colors || (img->face_foreground == foreground
                               && img->face_background == background)))
       break;
+  
   return img;
 }
 
@@ -1649,7 +1650,7 @@ uncache_image (struct frame *f, Lisp_Object spec)
      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, sxhash (spec), 0, 0, true)))
+  while ((img = search_image_cache (f, spec, img_hash (spec), 0, 0, true)))
     {
       free_image (f, img);
       /* As display glyphs may still be referring to the image ID, we
@@ -2346,7 +2347,7 @@ lookup_image (struct frame *f, Lisp_Object spec, int face_id)
   eassert (valid_image_p (spec));
 
   /* Look up SPEC in the hash table of the image cache.  */
-  hash = sxhash (spec);
+  hash = img_hash (spec);
   img = search_image_cache (f, spec, hash, foreground, background, true);
   if (img && img->load_failed_p)
     {
diff --git a/src/lisp.h b/src/lisp.h
index 416c9b0cac..4ca5aeced1 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3595,6 +3595,7 @@ #define CONS_TO_INTEGER(cons, type, var)				\
 extern char *extract_data_from_object (Lisp_Object, ptrdiff_t *, ptrdiff_t *);
 EMACS_UINT hash_string (char const *, ptrdiff_t);
 EMACS_UINT sxhash (Lisp_Object);
+EMACS_UINT img_hash (Lisp_Object);
 Lisp_Object hashfn_eql (Lisp_Object, struct Lisp_Hash_Table *);
 Lisp_Object hashfn_equal (Lisp_Object, struct Lisp_Hash_Table *);
 Lisp_Object hashfn_user_defined (Lisp_Object, struct Lisp_Hash_Table *);


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: ghe@sdf.org,  Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Tue, 08 Dec 2020 20:36:47 +0100
> 
> +	case Lisp_String:
> +	  val = (EMACS_UINT) SSDATA (obj);

Why use the pointer?  Lisp strings can be relocated, and there could
be another string with the same contents.  Why not support those?

> @@ -1649,7 +1650,7 @@ uncache_image (struct frame *f, Lisp_Object spec)
>       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, sxhash (spec), 0, 0, true)))
> +  while ((img = search_image_cache (f, spec, img_hash (spec), 0, 0, true)))

Any reason you call img_hash in a loop with the same argument?

Last, but not least, a question: what kind of savings will this
produce, and why?

Thanks.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> +	case Lisp_String:
>> +	  val = (EMACS_UINT) SSDATA (obj);
>
> Why use the pointer?  Lisp strings can be relocated, and there could
> be another string with the same contents.  Why not support those?

Yes, that's not a good ID for the string.  What's the proper way to get
the identity of the string object?

>> -  while ((img = search_image_cache (f, spec, sxhash (spec), 0, 0, true)))
>> +  while ((img = search_image_cache (f, spec, img_hash (spec), 0, 0, true)))
>
> Any reason you call img_hash in a loop with the same argument?

It's not me; it's search and replace.  And, no, there's no reason why
the sxhash in in that loop; it should be fixed regardless.

> Last, but not least, a question: what kind of savings will this
> produce, and why?

It depends on many different things, but between 0% and 20% in my test
cases.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> It depends on many different things, but between 0% and 20% in my test
> cases.

I mean, it's between 0% to 50% faster, but the 50% test was somewhat
unrealistic (many identical images in the same buffer).  In a more
realistic, but very image-heavy eww-like buffer, it's about 15% faster
scrolling through the buffer.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I mean, it's between 0% to 50% faster, but the 50% test was somewhat
> unrealistic (many identical images in the same buffer).  In a more
> realistic, but very image-heavy eww-like buffer, it's about 15% faster
> scrolling through the buffer.

img_hash 32.1s
sxhash 42.0s

with this test:

(defun bench ()
  (switch-to-buffer "images")
  (erase-buffer)
    (dotimes (i 200)
      (let ((file (format "/tmp/P%d.JPG" (+ i 1320800))))
	(when (file-exists-p file)
	  (insert-image
	   (create-image
	    (with-temp-buffer
	      (set-buffer-multibyte nil)
	      (insert-file-contents-literally file)
	      (buffer-string))
	    'jpeg t :scale 1)))
	(insert "\n\n")))
  (goto-char (point-min))
  (benchmark-progn
    (dotimes (i 5)
      (goto-char (point-min))
      (while (not (eobp))
	(sit-for 0.01)
	(forward-line 2)))))

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Alfred M. Szmidt]

   +	case_Lisp_Int:

That probobly doesn't do what you want.

Re: Redisplay slower in Emacs 28 than Emacs 27

[Stefan Monnier]

Lars Ingebrigtsen [2020-12-08 20:12:59] wrote:
> :data image, current sxhash, Fequal: 6.4s.
> :data image, NOOP sxhash, Fequal: 3.4s.

Maybe it's time we speed up `hash_string`.
There's a lot of room for improvement there, AFAICT, e.g.:
- work on `EMACS_UINT` chunks instead of `char` chunks.
- Bound the amount of work (e.g. do a max of, say, 10 chunks, equally
  spaced throughout the string).


        Stefan

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

"Alfred M. Szmidt" <ams@gnu.org> writes:

>    +	case_Lisp_Int:
>
> That probobly doesn't do what you want.

#define case_Lisp_Int case Lisp_Int0: case Lisp_Int1

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Alfred M. Szmidt]

   >    +	case_Lisp_Int:
   >
   > That probobly doesn't do what you want.

   #define case_Lisp_Int case Lisp_Int0: case Lisp_Int1

D'oh...

Re: Redisplay slower in Emacs 28 than Emacs 27

[Stefan Monnier]

>> :data image, current sxhash, Fequal: 6.4s.
>> :data image, NOOP sxhash, Fequal: 3.4s.
>
> Maybe it's time we speed up `hash_string`.
> There's a lot of room for improvement there, AFAICT, e.g.:
> - work on `EMACS_UINT` chunks instead of `char` chunks.
> - Bound the amount of work (e.g. do a max of, say, 10 chunks, equally
>   spaced throughout the string).

E.g.


        Stefan


diff --git a/src/fns.c b/src/fns.c
index e4c9acc316..23d24ef4db 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -4525,18 +4526,40 @@ #define SXHASH_MAX_LEN   7
 EMACS_UINT
 hash_string (char const *ptr, ptrdiff_t len)
 {
-  char const *p = ptr;
-  char const *end = p + len;
-  unsigned char c;
-  EMACS_UINT hash = 0;
-
-  while (p != end)
+  if (len < 16)
     {
-      c = *p++;
-      hash = sxhash_combine (hash, c);
+      char const *p = ptr;
+      char const *end = p + len;
+      EMACS_UINT hash = len;
+
+      while (p < end)
+        {
+          unsigned char c = *p++;
+          hash = sxhash_combine (hash, c);
+        }
+
+      return hash;
     }
+  else
+    {
+      EMACS_UINT const *p   = (EMACS_UINT const *) ptr;
+      EMACS_UINT const *end = (EMACS_UINT const *) (ptr + len);
+      EMACS_UINT hash = len;
+      /* At most 8 steps.  We could reuse SXHASH_MAX_LEN, of course,
+       * but dividing by 8 is cheaper.  */
+      ptrdiff_t step = max (1, (end - p) >> 3);
+
+      /* Beware: `end` might be unaligned, so `p < end` is not always the same
+       * as `p <= end - 1`.  */
+      while (p <= end - 1)
+        {
+          EMACS_UINT c = *p;
+          p += step;
+          hash = sxhash_combine (hash, c);
+        }
 
-  return hash;
+      return hash;
+    }
 }
 
 /* Return a hash for string PTR which has length LEN.  The hash

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> E.g.
>
>         Stefan
>
> diff --git a/src/fns.c b/src/fns.c
> index e4c9acc316..23d24ef4db 100644

Nice; with that, my benchmark runs as fast as with my img_hash version.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Stefan Monnier]

> Nice; with that, my benchmark runs as fast as with my img_hash version.

Thanks, pushed to master,


        Stefan

Re: Redisplay slower in Emacs 28 than Emacs 27

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Nice; with that, my benchmark runs as fast as with my img_hash version.
>
> Thanks, pushed to master,

Great.  I've now restored the Fequal in the cache lookup, so image cache
semantics should now be back to what they were in Emacs 27.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  ghe@sdf.org,  emacs-devel@gnu.org
> Date: Tue, 08 Dec 2020 18:10:49 -0500
> 
> > Nice; with that, my benchmark runs as fast as with my img_hash version.
> 
> Thanks, pushed to master,

Btw, any particular reason for the cutoff of 16 bytes to switch to the
"large chunk" branch?

Re: Redisplay slower in Emacs 28 than Emacs 27

[Stefan Monnier]

> Btw, any particular reason for the cutoff of 16 bytes to switch to the
> "large chunk" branch?

The reason is that I felt it looked good ;-)
Well, not only: the "large chunk" branch operates 8bytes at a time on
64bit systems, so if we use it on a 15B string it will only consult the
first 8B which I thought wasn't good enough, whereas I felt that
ignoring the last 7B of a 23B string was acceptable.


        Stefan

Re: Redisplay slower in Emacs 28 than Emacs 27

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: larsi@gnus.org,  ghe@sdf.org,  emacs-devel@gnu.org
> Date: Wed, 09 Dec 2020 16:01:41 -0500
> 
> > Btw, any particular reason for the cutoff of 16 bytes to switch to the
> > "large chunk" branch?
> 
> The reason is that I felt it looked good ;-)
> Well, not only: the "large chunk" branch operates 8bytes at a time on
> 64bit systems, so if we use it on a 15B string it will only consult the
> first 8B which I thought wasn't good enough, whereas I felt that
> ignoring the last 7B of a 23B string was acceptable.

Then perhaps we should look for the optimal cutoff by benchmarking
with different values?

Re: Redisplay slower in Emacs 28 than Emacs 27

[Stefan Monnier]

>> > Btw, any particular reason for the cutoff of 16 bytes to switch to the
>> > "large chunk" branch?
>> The reason is that I felt it looked good ;-)
>> Well, not only: the "large chunk" branch operates 8bytes at a time on
>> 64bit systems, so if we use it on a 15B string it will only consult the
>> first 8B which I thought wasn't good enough, whereas I felt that
>> ignoring the last 7B of a 23B string was acceptable.
> Then perhaps we should look for the optimal cutoff by benchmarking
> with different values?

If someone feels like it, I won't get in the way.  Personally, I don't
think "optimal" is needed here.

BTW, we could also get rid of the "old, slow code" and instead improve
the fast code so it doesn't throw away the last <8 bytes.  If done
right, it would likely also speed up the case of strings shorter than
16 bytes.

If we presume that `p` itself is naturally aligned (which I believe
should indeed always be the case), then we can safely use a word-sized
read at the end (it will read some trailing garbage bytes, but won't
risk hitting a memory protection check) and then "mask out" those
trailing bytes.  But that masking requires endian-dependent code, so
I refrained from going down that road.  I guess if we could arrange for
those trailing bytes to always be the same (e.g. always NUL), then we
wouldn't even need to do that masking, but AFAIK while we do make sure
there's always a trailing NUL right after the end of our strings, we
don't ensure that there are trailing NULs right up to the next multiple
of 4-or-8 address.

Admittedly, big-endians lost the war and are a disappearing breed
nowadays, but they still haven't quite disappeared, sadly [ I was
rooting for the big-endians, but given where we're at, I'd rather see
big-endians be a thing of the past.  ]


        Stefan