[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH]: Fix memory leak in diff

From: Peter N. Lundblad <peter_at_famlundblad.se>
Date: 2005-11-24 08:40:47 CET

On Wed, 23 Nov 2005, Daniel Berlin wrote:

> One of my users noticed that if you specify files seperately to svn
> diff, the memory usage goes through the roof.
>
> IE if you do svn diff *.c */*.c on a gcc checkout in the gcc dir, it
> uses more than 300 meg of memory.
>
> This is caused by not actually passing the per-iteration pool to our
> children.
Whoa! This was a nice one:-)

> [[[
>
> Fix memory leak when diff is given multiple files on the command line.
>
> * subversion/clients/cmdlne/diff-cmd.c
"cmdlne":-)

(Do you know there is svn-dev.el with an svn-log-message command?)

> Index: subversion/clients/cmdline/diff-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/diff-cmd.c (revision 17493)
> +++ subversion/clients/cmdline/diff-cmd.c (working copy)
> @@ -49,7 +49,7 @@ svn_cl__diff (apr_getopt_t *os,
> apr_file_t *outfile, *errfile;
> apr_status_t status;
> const char *old_target, *new_target;
> - apr_pool_t *subpool;
> + apr_pool_t *iterpool;

We use subpool for iterations all over the place, so I don't really
see the point. But if someone feels this is clearer, then I'm all for
it.

> svn_boolean_t pegged_diff = FALSE;
> int i;
>
> @@ -172,7 +172,7 @@ svn_cl__diff (apr_getopt_t *os,
>
> svn_opt_push_implicit_dot_target (targets, pool);
>
> - subpool = svn_pool_create (pool);
> + iterpool = svn_pool_create (pool);
> for (i = 0; i < targets->nelts; ++i)
> {
> const char *path = APR_ARRAY_IDX (targets, i, const char *);
> @@ -180,9 +180,9 @@ svn_cl__diff (apr_getopt_t *os,
>
> if (! pegged_diff)
> {
> - svn_pool_clear (subpool);
> - target1 = svn_path_join (old_target, path, subpool);
> - target2 = svn_path_join (new_target, path, subpool);
> + svn_pool_clear (iterpool);

Why is this pool clearing inside the if statement? This seems to leave
the leak in the pegged_diff case.

...
> }
> else
> {
> @@ -205,7 +205,7 @@ svn_cl__diff (apr_getopt_t *os,
> svn_opt_revision_t peg_revision;
>
> /* First check for a peg revision. */
> - SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, path, pool));
> + SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, path, iterpool));

I don't know if this line slips over the 80 cols limit without the
mailer and diff prefixes.

Else, looks great!

thanks,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Nov 24 08:41:38 2005

This is an archived mail posted to the Subversion Dev mailing list.