You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Robert Pluim <rp...@bigfoot.com> on 2003/02/27 17:04:20 UTC
[PATCH] make mod_dav_svn leak a little less
Besides being pretty slow, checkout over dav makes my httpd grow
enormously. The following patch ameliorates that somewhat (http grows
to ~10Meg rather than 120 for me now). It passes the testsuite.
Any suggestions on good places to look for performance improvements
over dav?
Robert
* svn/subversion/mod_dav_svn/repos.c (dav_svn_getetag): Use the
subpool provided in the resource if it's valid. Makes checkout
over dav use a lot less memory.
Re: [PATCH] make mod_dav_svn leak a little less
Posted by Sander Roobol <ph...@wanadoo.nl>.
This patch has been filed as issue #1179.
Sander
On Thu, Feb 27, 2003 at 06:04:20PM +0100, Robert Pluim wrote:
Content-Description: message body text
> Besides being pretty slow, checkout over dav makes my httpd grow
> enormously. The following patch ameliorates that somewhat (http grows
> to ~10Meg rather than 120 for me now). It passes the testsuite.
>
> Any suggestions on good places to look for performance improvements
> over dav?
>
> Robert
>
> * svn/subversion/mod_dav_svn/repos.c (dav_svn_getetag): Use the
> subpool provided in the resource if it's valid. Makes checkout
> over dav use a lot less memory.
>
> Index: repos.c
> ===================================================================
> --- repos.c (revision 5088)
> +++ repos.c (working copy)
> @@ -1613,7 +1613,13 @@
> {
> svn_error_t *serr;
> svn_revnum_t created_rev;
> + apr_pool_t * p;
>
> + if (resource->info->pool)
> + p = resource->info->pool;
> + else
> + p = resource->pool;
> +
> /* if the resource doesn't exist, isn't a simple REGULAR or VERSION
> resource, or it is a Baseline, then it has no etag. */
> /* ### we should assign etags to all resources at some point */
> @@ -1627,15 +1633,15 @@
>
> if ((serr = svn_fs_node_created_rev(&created_rev, resource->info->root.root,
> resource->info->repos_path,
> - resource->pool)))
> + p)))
> {
> /* ### what to do? */
> return "";
> }
>
> - return apr_psprintf(resource->pool, "\"%" SVN_REVNUM_T_FMT "/%s\"",
> + return apr_psprintf(p, "\"%" SVN_REVNUM_T_FMT "/%s\"",
> created_rev,
> - apr_xml_quote_string(resource->pool,
> + apr_xml_quote_string(p,
> resource->info->repos_path, 1));
> }
>
Content-Description: .signature
>
> --
> Mobile: +33 6 19 49 54 48
> Tel: +33 4 92 94 05 94
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] make mod_dav_svn leak a little less
Posted by Robert Pluim <rp...@bigfoot.com>.
Greg Stein writes:
> >
> > Hmm, is the signature of dav_svn_getetag cast in stone?
>
> Right. The ideal is that more mod_dav hooks get a pool passed to them.
>
> The signatures are not set in stone, but changing them requires versioning
> the interface. That might not be so pleasant, but is doable.
>
This occurred to me this morning as I was in the shower:
const char * dav_svn__getetag_internal (const dav_resource *,
apr_pool_t *pool);
const char * dav_svn_getetag(const dav_resource *resource)
{
return dav_svn__getetag_internal(resource, resource->pool);
}
dav_svn_insert_prop would then call
dav_svn__getetag_internal(resource, resource->info->pool), and the
official API could stay the same. Of course, this kind of wrapping
is horrible, but it's the best I can come up with before my morning
coffee ;-)
Robert
--
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] make mod_dav_svn leak a little less
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Feb 27, 2003 at 06:40:51PM +0100, Robert Pluim wrote:
> Justin Erenkrantz writes:
> > --On Thursday, February 27, 2003 6:04 PM +0100 Robert Pluim
> > <rp...@bigfoot.com> wrote:
> >
> > > - return apr_psprintf(resource->pool, "\"%" SVN_REVNUM_T_FMT "/%s\"",
> > > + return apr_psprintf(p, "\"%" SVN_REVNUM_T_FMT "/%s\"",
> > > created_rev,
> > > - apr_xml_quote_string(resource->pool,
> > > + apr_xml_quote_string(p,
> > > resource->info->repos_path, 1));
> >
> > This may be bogus because resource->info->pool may be cleaned up
> > before the response is written. The test suite has no coverage
> > over any of the lifetime issues and probably wouldn't check the
> > ETag value for corruption. -- justin
bleh :-(
> The case that was causing the problem was when dav_svn_insert_prop
> calls dav_svn_getetag, since that causes temporary stuff to be
> allocated out of the response pool rather than the subpool in
> resource->info->pool. In this particular case, the return value of
> dav_svn_getetag is copied into the response pool, so I think it's ok
> here. I can't speak to the other cases until I find them :-)
>
> Hmm, is the signature of dav_svn_getetag cast in stone?
Right. The ideal is that more mod_dav hooks get a pool passed to them.
The signatures are not set in stone, but changing them requires versioning
the interface. That might not be so pleasant, but is doable.
If somebody wants to dig into creating a new API version for mod_dav, then
let me know, and we can start that discussion. The facilities are present,
so it is really a design discussion from there.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] make mod_dav_svn leak a little less
Posted by Robert Pluim <rp...@bigfoot.com>.
Justin Erenkrantz writes:
> --On Thursday, February 27, 2003 6:04 PM +0100 Robert Pluim
> <rp...@bigfoot.com> wrote:
>
> > - return apr_psprintf(resource->pool, "\"%" SVN_REVNUM_T_FMT "/%s\"",
> > + return apr_psprintf(p, "\"%" SVN_REVNUM_T_FMT "/%s\"",
> > created_rev,
> > - apr_xml_quote_string(resource->pool,
> > + apr_xml_quote_string(p,
> > resource->info->repos_path, 1));
>
> This may be bogus because resource->info->pool may be cleaned up
> before the response is written. The test suite has no coverage
> over any of the lifetime issues and probably wouldn't check the
> ETag value for corruption. -- justin
The case that was causing the problem was when dav_svn_insert_prop
calls dav_svn_getetag, since that causes temporary stuff to be
allocated out of the response pool rather than the subpool in
resource->info->pool. In this particular case, the return value of
dav_svn_getetag is copied into the response pool, so I think it's ok
here. I can't speak to the other cases until I find them :-)
Hmm, is the signature of dav_svn_getetag cast in stone?
Robert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] make mod_dav_svn leak a little less
Posted by Justin Erenkrantz <je...@apache.org>.
--On Thursday, February 27, 2003 6:04 PM +0100 Robert Pluim
<rp...@bigfoot.com> wrote:
> - return apr_psprintf(resource->pool, "\"%" SVN_REVNUM_T_FMT "/%s\"",
> + return apr_psprintf(p, "\"%" SVN_REVNUM_T_FMT "/%s\"",
> created_rev,
> - apr_xml_quote_string(resource->pool,
> + apr_xml_quote_string(p,
> resource->info->repos_path, 1));
This may be bogus because resource->info->pool may be cleaned up before the
response is written. The test suite has no coverage over any of the lifetime
issues and probably wouldn't check the ETag value for corruption. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org