From: Paul Eggert <eggert@cs.ucla.edu>
To: Lars Magne Ingebrigtsen <larsi@gnus.org>
Cc: emacs-devel@gnu.org
Subject: Re: trunk r113947: * image.c: Fix animation cache signature memory leak.
Date: Mon, 19 Aug 2013 12:46:11 -0700 [thread overview]
Message-ID: <52127603.5030809@cs.ucla.edu> (raw)
In-Reply-To: <m3fvu5obmc.fsf@stories.gnus.org>
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);
next prev parent reply other threads:[~2013-08-19 19:46 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <E1VBJTG-0003rv-7A@vcs.savannah.gnu.org>
2013-08-19 15:22 ` trunk r113947: * image.c: Fix animation cache signature memory leak Lars Magne Ingebrigtsen
2013-08-19 16:16 ` Paul Eggert
2013-08-19 16:19 ` Lars Magne Ingebrigtsen
2013-08-19 16:24 ` Paul Eggert
2013-08-19 16:40 ` Lars Magne Ingebrigtsen
2013-08-19 16:56 ` Paul Eggert
2013-08-19 17:04 ` Lars Magne Ingebrigtsen
2013-08-19 19:46 ` Paul Eggert [this message]
2013-08-19 19:51 ` Lars Magne Ingebrigtsen
2013-08-19 20:52 ` Paul Eggert
2013-08-19 20:56 ` Lars Magne Ingebrigtsen
2013-08-19 20:55 ` Lars Magne Ingebrigtsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52127603.5030809@cs.ucla.edu \
--to=eggert@cs.ucla.edu \
--cc=emacs-devel@gnu.org \
--cc=larsi@gnus.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).