From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Lars Magne Ingebrigtsen Newsgroups: gmane.emacs.devel Subject: Re: trunk r113947: * image.c: Fix animation cache signature memory leak. Date: Mon, 19 Aug 2013 17:22:01 +0200 Message-ID: References: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1376925757 6364 80.91.229.3 (19 Aug 2013 15:22:37 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 19 Aug 2013 15:22:37 +0000 (UTC) Cc: emacs-devel@gnu.org To: Paul Eggert Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Aug 19 17:22:39 2013 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1VBRHu-00088i-FZ for ged-emacs-devel@m.gmane.org; Mon, 19 Aug 2013 17:22:34 +0200 Original-Received: from localhost ([::1]:43670 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VBRHu-0005le-2b for ged-emacs-devel@m.gmane.org; Mon, 19 Aug 2013 11:22:34 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:47685) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VBRHj-0005ZE-Bt for emacs-devel@gnu.org; Mon, 19 Aug 2013 11:22:31 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VBRHc-0008G5-Aw for emacs-devel@gnu.org; Mon, 19 Aug 2013 11:22:23 -0400 Original-Received: from hermes.netfonds.no ([80.91.224.195]:53589) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VBRHc-0008FM-27 for emacs-devel@gnu.org; Mon, 19 Aug 2013 11:22:16 -0400 Original-Received: from cm-84.215.51.58.getinternet.no ([84.215.51.58] helo=stories.gnus.org) by hermes.netfonds.no with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.72) (envelope-from ) id 1VBRHO-0002ru-7o; Mon, 19 Aug 2013 17:22:02 +0200 Face: iVBORw0KGgoAAAANSUhEUgAAADAAAAAwBAMAAAClLOS0AAAAKlBMVEWTb3YVDArj1uj5+f3z 7/rRvtFuPDf9/v7////t5vT+//6miZfAp7qIWlvb856/AAACKElEQVQ4jW3SsY6bQBAA0KnSEBc0 +YCNUUQNke9ag5RsSzRSpNDQbOeKRPhqR8JyiX2KVvmApKRLsbg5iWq3cOWOf8ks5/PZmClsNG+Y 2Vkb1n2syrv1dYD92Gzy9Wx5Az/w6aMxX4dvLNe/walr+HcLHmqD+OUGij/Fw/uiKK9nlLCsWMV8 v5IfBnDHeSwjKfmvK8hpxsYeeDChDGG4WJ/+u7ofhSDPOSzHIIrmY1CGcVyNQR5zyS6hfP5ahXPJ vEtYnV7gtDKOQMgZ88zlcVenTtUQnmesYu4xNLcLfg8lwzEI4oryBj4PIeSPiK2GaJAvQ+khpgp2 g9V/ct92UjAtrl94oNGt0Qr88/T+tOV95aUpWvh0sV9Z5D20CiA5w4zSQcArlhq0sD01KqKA8qEv H7G1gNvTsfIoCsKI+3KOaUqnal+G5NtdWMlq6lct6m8WtjP7LyyDWErGmM8YJrQ4mGQaFcU6zyNK 07WyBGlBtYcG/V0U0O8vK7oKigS91NQATct8uQvorj17FcYklG/qCTgGqTGP7fH7oBvUuj4StEjE vCetKBprWsMRDsoWIepG13ASXU8EdA49tmjqt9ABAIFq6n2XweLg2DLdOUfRHTpQ1LGeLAiozlFG iYMQi07YMgVHkYEtO1CR6DJXkBHUBC4BBai9EK7rkkHtwKGHRddRuzeuBdd9R50nC5ERiM5a5p7C drA1BLazeAWK7Ayv+R7cMXiJ/66uE2jh3QDTAAAAAElFTkSuQmCC X-Now-Playing: The Cure's _Seventeen Seconds (1)_: "A Reflection" X-Hashcash: 1:23:130819:emacs-devel@gnu.org::Rs4ZoBHK9Y4FsY1K:0000000000000000000000000000000000000000009Ikj X-Hashcash: 1:23:130819:eggert@cs.ucla.edu::LU0zRKtSbDkBxxDn:0000000000000000000000000000000000000000000ESWU In-Reply-To: (Paul Eggert's message of "Mon, 19 Aug 2013 07:01:46 +0000") User-Agent: Gnus/5.130008 (Ma Gnus v0.8) Emacs/24.3.50 (gnu/linux) X-MailScanner-ID: 1VBRHO-0002ru-7o MailScanner-NULL-Check: 1377530522.86361@6uV7Wc+w2M7SZOcxsbftcQ X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 80.91.224.195 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:162878 Archived-At: Paul Eggert 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