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

[Lars Magne Ingebrigtsen]

Paul Eggert <address@hidden> 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