From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: joakim@verona.se Newsgroups: gmane.emacs.devel Subject: Re: Using the ImageMagick backend seems to leak memory Date: Tue, 11 Jan 2011 20:35:52 +0100 Message-ID: References: <87sjx6zczl.fsf@member.fsf.org> <30611405.post@talk.nabble.com> <874o9l86eb.fsf@member.fsf.org> <87zkr7nxo8.fsf@member.fsf.org> <8739oz8dei.fsf@member.fsf.org> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: dough.gmane.org 1294774572 12912 80.91.229.12 (11 Jan 2011 19:36:12 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Tue, 11 Jan 2011 19:36:12 +0000 (UTC) Cc: Emacs-devel@gnu.org, Jashy , Miles Bader To: Tassilo Horn Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Jan 11 20:36:07 2011 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Pck0j-0001HS-83 for ged-emacs-devel@m.gmane.org; Tue, 11 Jan 2011 20:36:05 +0100 Original-Received: from localhost ([127.0.0.1]:36065 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pck0i-0007Jz-BQ for ged-emacs-devel@m.gmane.org; Tue, 11 Jan 2011 14:36:04 -0500 Original-Received: from [140.186.70.92] (port=39549 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pck0c-0007Jt-UM for Emacs-devel@gnu.org; Tue, 11 Jan 2011 14:35:59 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pck0c-0002bB-0B for Emacs-devel@gnu.org; Tue, 11 Jan 2011 14:35:58 -0500 Original-Received: from iwfs.imcode.com ([82.115.149.64]:46505 helo=gate.verona.se) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pck0a-0002al-9u; Tue, 11 Jan 2011 14:35:56 -0500 Original-Received: from localhost.localdomain (IDENT:1005@localhost [127.0.0.1]) by gate.verona.se (8.13.4/8.11.4) with ESMTP id p0BJZqA7008596; Tue, 11 Jan 2011 20:35:52 +0100 In-Reply-To: <8739oz8dei.fsf@member.fsf.org> (Tassilo Horn's message of "Tue, 11 Jan 2011 20:10:29 +0100") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4-2.6 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:134453 Archived-At: Tassilo Horn writes: > joakim@verona.se writes: > > Hi Joakim, > >> I've looked at this a little bit further. >> >> The error is that the image read with ReadImage is cloned by >> NewMagickWandFromImage not refered as I believed when I wrote the >> code. >> >> The code was written that way to avoid another memory problem I >> experienced with image bundles. If the current strategy remains, >> DestroyImage(im_image); should be the right fix. > > I've tested that, and now memory is freed as expected. Still, when I > used doc-view to view a 30 pages PDF (aka 30 large PNGs), the emacs > process started with ~11 MB when showing the first page and was at 12.2 > MB after viewing all pages in sequence, killing the doc-view buffer, and > doing (clear-image-cache) and (garbage-collect). But that might be no > real leak. > >> However, the strategy should probably be changed to avoid the cloning. >> This will result in a more complex code path. > > Well, you are the ImageMagick master. ;-) > > Anyway, should I commit the following patch which includes your > DestroyImage() call that fixes the issue, and I additionally surrounded > image loading with calls to MagickWandGenesis()/MagickWandTerminus(), > because that's the way the API is used throughout the example codes in > the MagickWand API docs? The patch seems fine. Im not entirely sure about genesis/terminus though, I interpreted the docs to mean genesis=library load. > > === modified file 'src/image.c' > --- src/image.c 2011-01-07 22:33:32 +0000 > +++ src/image.c 2011-01-11 19:02:33 +0000 > @@ -7521,6 +7521,9 @@ > image. Interface :index is same as for GIF. First we "ping" the > image to see how many sub-images it contains. Pinging is faster > than loading the image to find out things about it. */ > + > + /* MagickWandGenesis() initializes the imagemagick library. */ > + MagickWandGenesis (); > image = image_spec_value (img->spec, QCindex, NULL); > ino = INTEGERP (image) ? XFASTINT (image) : 0; > ping_wand = NewMagickWand (); > @@ -7549,6 +7552,7 @@ > img->data.lisp_val)); > > DestroyMagickWand (ping_wand); > + > /* Now, after pinging, we know how many images are inside the > file. If its not a bundle, just one. */ > > @@ -7566,6 +7570,7 @@ > if (im_image != NULL) > { > image_wand = NewMagickWandFromImage (im_image); > + DestroyImage(im_image); > status = MagickTrue; > } > else > @@ -7576,7 +7581,7 @@ > image_wand = NewMagickWand (); > status = MagickReadImageBlob (image_wand, contents, size); > } > - image_error ("im read failed", Qnil, Qnil); > + > if (status == MagickFalse) goto imagemagick_error; > > /* If width and/or height is set in the display spec assume we want > @@ -7805,11 +7810,13 @@ > > /* Final cleanup. image_wand should be the only resource left. */ > DestroyMagickWand (image_wand); > + MagickWandTerminus (); > > return 1; > > imagemagick_error: > DestroyMagickWand (image_wand); > + MagickWandTerminus (); > /* TODO more cleanup. */ > image_error ("Error parsing IMAGEMAGICK image `%s'", img->spec, Qnil); > return 0; > @@ -8681,8 +8688,6 @@ > #if defined (HAVE_IMAGEMAGICK) > if (EQ (type, Qimagemagick)) > { > - /* MagickWandGenesis() initializes the imagemagick library. */ > - MagickWandGenesis (); > return CHECK_LIB_AVAILABLE (&imagemagick_type, init_imagemagick_functions, > libraries); > } > > > Bye, > Tassilo -- Joakim Verona