Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

[Lars Magne Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

>   * image.c: Fix animation cache signature memory leak.

Thank you for fixing that.

However:

>   Fix some other minor performance problems while we're at it.
>   (imagemagick_create_cache): Clear just the members that
>   need clearing.

This is the fix -- not to call xzalloc, because then we ... just don't
have to set update_time and signature twice.

> -  struct animation_cache *cache = xzalloc (sizeof *cache);
> +  struct animation_cache *cache = xmalloc (sizeof *cache);
>    cache->signature = signature;
> -  cache->update_time = current_emacs_time ();
> +  cache->wand = 0;
> +  cache->index = 0;
> +  cache->next = 0;

That's a major speedup -- creating an animation cache entry is done once
per animated image.  Instead you make the code more fragile and longer,
so that it'll have to be updated if we add more fields.

Seriously?  That's a performance problem?  I repeat, once per animated
image.  You'll be lucky if that happens once per hour.

>   (imagemagick_prune_animation_cache, imagemagick_get_animation_cache):
>   Simplify by using pointer-to-pointer instead of a prev pointer.

Yeah, that was a lot simpler:

> -  if (! cache)
> -    {
> -      animation_cache = imagemagick_create_cache (signature);
> -      return animation_cache;
> -    }
> -
> -  while (strcmp(signature, cache->signature) &&
> -	 cache->next)
> -    cache = cache->next;
> -
> -  if (strcmp(signature, cache->signature))
> -    {
> -      cache->next = imagemagick_create_cache (signature);
> -      return cache->next;
> +  for (pcache = &animation_cache; *pcache; pcache = &cache->next)
> +    {
> +      cache = *pcache;
> +      if (! cache)
> +	{
> +	  *pcache = cache = imagemagick_create_cache (signature);
> +	  break;
> +	}
> +      if (strcmp (signature, cache->signature) == 0)
> +	{
> +	  DestroyString (signature);
> +	  break;
> +	}

The new code is no shorter, but it's more difficult to understand.  The
pointer to a pointer thing is quite clever.  Perhaps too clever, because
you obviously didn't understand it yourself, and you didn't test it,
because it segfaulted the first time it was called.

imagemagick_prune_animation_cache was an improvement, though.

But my point is:  Seriously?  Are you doing these rewrites just to
create maximum obfuscation or something?  If it sounds like I'm pissed,
it's because I am, because I had to spend an hour debugging this
segfault instead of getting on with my job.

-- 
(domestic pets only, the antidote for overdose, milk.)
  No Gnus T-Shirt for sale: http://ingebrigtsen.no/no.php
  and http://lars.ingebrigtsen.no/2013/08/twenty-years-of-september.html

Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

[Paul Eggert]

Lars Magne Ingebrigtsen wrote:
 
> Seriously?  That's a performance problem?

I did say it was minor.  :-)  I think it's clearer if the code
initializes things just once, but if you think it clearer the other
way please feel free to revert it.

> you didn't test it,
> because it segfaulted the first time it was called.

I did test it, but evidently not with enough test cases.
Sorry about the bug.

To avoid such problems in the future, should we funnel future
image.c changes through you?  Here's a tiny style issue that
should be fixed at some point: the version you inserted has
"strcmp(" in a couple of places where it should be "strcmp (".

Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

[Lars Magne Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

>> you didn't test it,
>> because it segfaulted the first time it was called.
>
> I did test it, but evidently not with enough test cases.
> Sorry about the bug.

But it would crash with any (animated) image, because it never allocated
a cache.  If I read the code correctly.  :-)

> To avoid such problems in the future, should we funnel future
> image.c changes through you?

No no.

> Here's a tiny style issue that should be fixed at some point: the
> version you inserted has "strcmp(" in a couple of places where it
> should be "strcmp (".

No, please go ahead and fix.

-- 
(domestic pets only, the antidote for overdose, milk.)
  No Gnus T-Shirt for sale: http://ingebrigtsen.no/no.php
  and http://lars.ingebrigtsen.no/2013/08/twenty-years-of-september.html

Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

[Paul Eggert]

Lars Magne Ingebrigtsen wrote:

> But it would crash with any (animated) image, because it never allocated
> a cache.  If I read the code correctly.  :-)

Hmmm, well, then I must have tested it incorrectly.
There was a dancing image on my screen, anyway....

> No, please go ahead and fix.

Done.  Again, sorry about the earlier bug.

Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

[Lars Magne Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Hmmm, well, then I must have tested it incorrectly.
> There was a dancing image on my screen, anyway....

But this is the code.  animation_cache starts off as NULL.  That means
that *pcache is NULL, right?  So the for loop never happens.  So cache
is still NULL, and then we dereference that and blow up.

Or am I misreading this completely?  It wouldn't be the first time...

static struct animation_cache *
imagemagick_get_animation_cache (MagickWand *wand)
{
  char *signature = MagickGetImageSignature (wand);
  struct animation_cache *cache;
  struct animation_cache **pcache = &animation_cache;

  imagemagick_prune_animation_cache ();
  cache = animation_cache;

  for (pcache = &animation_cache; *pcache; pcache = &cache->next)
    {
      cache = *pcache;
      if (! cache)
	{
	  *pcache = cache = imagemagick_create_cache (signature);
	  break;
	}
      if (strcmp (signature, cache->signature) == 0)
	{
	  DestroyString (signature);
	  break;
	}
    }

  cache->update_time = current_emacs_time ();
  return cache;
}


-- 
(domestic pets only, the antidote for overdose, milk.)
  No Gnus T-Shirt for sale: http://ingebrigtsen.no/no.php
  and http://lars.ingebrigtsen.no/2013/08/twenty-years-of-september.html

Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

[Paul Eggert]

Lars Magne Ingebrigtsen wrote:
> Or am I misreading this completely?

No, you got it right.  I expect that I ran the wrong
executable when I thought I was testing the new code.

My code should have looked like the following, I expect.
The advantage of doing it this way is that it calls
imagemagick_create_cache in just one place, and strcmp
in just one place, and refers to 'animation_cache' in
just one place, and updates cache->update time in just
one place, and overall (to my mind anyway) is easier to
understand than the duplications in the current code.
But it's only a minor improvement.

static struct animation_cache *
imagemagick_get_animation_cache (MagickWand *wand)
{
  char *signature = MagickGetImageSignature (wand);
  struct animation_cache **pcache = &animation_cache;

  imagemagick_prune_animation_cache ();

  while (1)
    {
      struct animation_cache *cache = *pcache;
      if (! cache)
	{
          *pcache = cache = imagemagick_create_cache (signature);
          break;
        }
      if (strcmp (signature, cache->signature) == 0)
	{
          DestroyString (signature);
          break;
        }
      pcache = &cache->next;
    }

  cache->update_time = current_emacs_time ();
  return cache;
}

By the way, did you know that each animation signature
contains 4 KiB of wasted memory?  It's ImageMagick
code that's wasting it.  Dunno if that's worth tuning.

Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

[Lars Magne Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> By the way, did you know that each animation signature
> contains 4 KiB of wasted memory?  It's ImageMagick
> code that's wasting it.  Dunno if that's worth tuning.

I tried looking at the ImageMagick sources and it made my head swim.
The signature function calls AquireString, which eventually calls
AcquireMagickMemory, which manages a pool of memory.  Probably to avoid
having to call malloc a lot.

But if that can be tuned, then it would probably be nice to get rid of
the extra memory.  But it might not make much difference.  After all, we
stick the wand into the cache, and the wand is going to be a lot bigger.

Actually, calling the cache pruning function now and then would probably
be a good idea?  I'm not sure where it should be called from, though.
The idle timer?

-- 
(domestic pets only, the antidote for overdose, milk.)
  No Gnus T-Shirt for sale: http://ingebrigtsen.no/no.php
  and http://lars.ingebrigtsen.no/2013/08/twenty-years-of-september.html

Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

[Paul Eggert]

Lars Magne Ingebrigtsen wrote:

> calling the cache pruning function now and then would probably
> be a good idea?  I'm not sure where it should be called from, though.
> The idle timer?

Sounds plausible, though I'm not an idle-timer expert.

> if that can be tuned, then it would probably be nice to get rid of
> the extra memory.

Sure, that's easy.  Something like this, perhaps?  What images do
you use to test this sort of thing on?

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2013-08-19 17:56:58 +0000
+++ src/ChangeLog	2013-08-19 19:33:19 +0000
@@ -1,3 +1,19 @@
+2013-08-19  Paul Eggert  <eggert@cs.ucla.edu>
+
+	* image.c: Shrink memory needed for animation cache.
+	(SIGNATURE_DIGESTSIZE): New constant.
+	(struct animation_cache): Make 'signature' a fixed size array of bytes.
+	(imagemagick_create_cache): Copy the signature.  This saves
+	several KB of memory that ImageMagick wastes per signature.
+	Don't bother updating the update_time, as the caller does that now.
+	(imagemagick_prune_animation_cache): Don't destroy the signature, as
+	it's a fixed size struct member now.
+	(imagemagick_get_animation_cache): Always destroy the signature,
+	as it's now imagemagick_create_cache's responsibility to copy it.
+	Avoid duplicate calls to strcmp and to imagemagick_create_cache,
+	and use memcmp rather than strcmp.
+	eassert that ImageMagick returns a signature of the specified length.
+
 2013-08-19  Lars Magne Ingebrigtsen  <larsi@gnus.org>
 
 	* image.c (imagemagick_get_animation_cache): Don't segfault on

=== modified file 'src/image.c'
--- src/image.c	2013-08-19 17:56:58 +0000
+++ src/image.c	2013-08-19 19:33:19 +0000
@@ -7876,13 +7876,17 @@
    separate from the image cache, because the images may be scaled
    before display. */
 
+/* Size of ImageMagick image signatures, in bytes.  It's SHA-256 as a
+   hex string, so it's 256 bits represented via 4 bits per byte.  */
+enum { SIGNATURE_DIGESTSIZE = 256 / 4 };
+
 struct animation_cache
 {
-  char *signature;
   MagickWand *wand;
   int index;
   EMACS_TIME update_time;
   struct animation_cache *next;
+  char signature[SIGNATURE_DIGESTSIZE];
 };
 
 static struct animation_cache *animation_cache = NULL;
@@ -7891,11 +7895,10 @@
 imagemagick_create_cache (char *signature)
 {
   struct animation_cache *cache = xmalloc (sizeof *cache);
-  cache->signature = signature;
   cache->wand = 0;
   cache->index = 0;
   cache->next = 0;
-  cache->update_time = current_emacs_time ();
+  memcpy (cache->signature, signature, SIGNATURE_DIGESTSIZE);
   return cache;
 }
 
@@ -7914,7 +7917,6 @@
 	pcache = &cache->next;
       else
 	{
-	  DestroyString (cache->signature);
 	  if (cache->wand)
 	    DestroyMagickWand (cache->wand);
 	  *pcache = cache->next;
@@ -7928,24 +7930,22 @@
 {
   char *signature = MagickGetImageSignature (wand);
   struct animation_cache *cache;
+  struct animation_cache **pcache = &animation_cache;
 
+  eassert (strlen (signature) == SIGNATURE_DIGESTSIZE);
   imagemagick_prune_animation_cache ();
-  cache = animation_cache;
-
-  if (! cache)
-    {
-      animation_cache = imagemagick_create_cache (signature);
-      return animation_cache;
-    }
-
-  while (strcmp (signature, cache->signature) &&
-	 cache->next)
-    cache = cache->next;
-
-  if (strcmp (signature, cache->signature))
-    {
-      cache->next = imagemagick_create_cache (signature);
-      return cache->next;
+
+  while (1)
+    {
+      cache = *pcache;
+      if (! cache)
+	{
+          *pcache = cache = imagemagick_create_cache (signature);
+          break;
+        }
+      if (memcmp (signature, cache->signature, SIGNATURE_DIGESTSIZE) == 0)
+	break;
+      pcache = &cache->next;
     }
 
   DestroyString (signature);

Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

[Lars Magne Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Sure, that's easy.  Something like this, perhaps?

Sure.

> What images do you use to test this sort of thing on?

I usually pick something from cheezburger.  Like:

https://i.chzbgr.com/maxW500/7733068032/hCDD37D15/

-- 
(domestic pets only, the antidote for overdose, milk.)
  No Gnus T-Shirt for sale: http://ingebrigtsen.no/no.php
  and http://lars.ingebrigtsen.no/2013/08/twenty-years-of-september.html

Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

[Paul Eggert]

Lars Magne Ingebrigtsen wrote:

> Sure.

OK, installed as trunk bzr 113953.

> https://i.chzbgr.com/maxW500/7733068032/hCDD37D15/

Ah, when I visit that as a file, and type RET,
the image is animated but this new image.c code
is bypassed, since HAVE_GIF is true on my platform.
I thought I'd taken that into account in my earlier
testing, by unsetting HAVE_GIF in config.h, but perhaps not.

Are you configuring with the GIF, PNG, etc. libraries
disabled?  I ask because animated PNGs don't work on my
copy, but I'm using libpng instead of ImageMagick for PNGs.

Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

[Lars Magne Ingebrigtsen]

I think what might really help with the speed is doing more caching and
a better liveliness analysis.  Or something.

First of all, every iteration we re-parse the "super-wand":

  if (filename)
    status = MagickReadImage (image_wand, filename);
  else
    {
      filename_hint = imagemagick_filename_hint (img->spec, hint_buffer);
      MagickSetFilename (image_wand, filename_hint);
      status = MagickReadImageBlob (image_wand, contents, size);
    }

If we stash that in the cache, too, then my guess would be that we'd see
a rather large speed-up.  We'd have to have a way to compute an ID for
the data, though.  I'd suggest just doing a sha256 or something over
contents and then perhaps using the same animation cache.  It'd need
some tweaks, though, because we don't want to cache non-animation
images, and we don't know whether they are or not before we've parsed
it.  :-)

The other thing is that imagemagick_compute_animated_image clones the
wand before putting it into the cache:

  cache->wand = CloneMagickWand (composite_wand)

This is because imagemagick_load_image later destroys the wand, so we
need a copy.  This could be avoided, of course, and my guess is that
this would also be a nice speed-up, because copying a wand must probably
entail copying the entire data.

If you could look at this, I think the ImageMagick animation could start
approaching the gif animation code speed-wise.  At present, it's much
slower, especially on large images.

This is the code snippet I use to test:

(url-retrieve "http://cdn.arwrath.com/1/148330.gif"
	      (lambda (&rest ignore)
		(search-forward "\n\n")
		(let ((image (create-image
			      (buffer-substring (point) (point-max))
			      'imagemagick t)))
		  (pop-to-buffer "*image*")
		  (erase-buffer)
		  (delete-all-overlays)
		  (put-image image (point) "*")
		  (image-animate image nil 60))))


-- 
(domestic pets only, the antidote for overdose, milk.)
  No Gnus T-Shirt for sale: http://ingebrigtsen.no/no.php
  and http://lars.ingebrigtsen.no/2013/08/twenty-years-of-september.html

Re: trunk r113947: * image.c: Fix animation cache signature memory leak.

[Lars Magne Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Are you configuring with the GIF, PNG, etc. libraries
> disabled?  I ask because animated PNGs don't work on my
> copy, but I'm using libpng instead of ImageMagick for PNGs.

You have to specify 'imagemagick to get Emacs to prefer that over the
"lesser" decoders.  See the code snippet I posted a minute ago.

-- 
(domestic pets only, the antidote for overdose, milk.)
  No Gnus T-Shirt for sale: http://ingebrigtsen.no/no.php
  and http://lars.ingebrigtsen.no/2013/08/twenty-years-of-september.html