You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kevin Pilch-Bisson <ke...@pilch-bisson.net> on 2002/06/30 05:03:22 UTC
mod_dav/mod_dav_svn PROPFINDS.
Hey,
Trying to help someone on IRC today, I imported the linux kernel source into a
repository. I then tried to check it out over dav and had problems. After
watching for a while, I noticed that whenever the checkout started a new
directory it would stop for quite a while, and the httpd memory usage would
grow. I soon found out that my svn client would time out before getting a
PROPFIND request back for the include/linux directory of the kernel sources.
The reason for this is that my machine starts thrashing, because the httpd
process trying to do the propfind grows huge (680MB before I killed it).(Note
by contrast that opening the directory in w3m took about 2 seconds, and I
didn't notice httpd's memory usage.
Tracing into this I found that mod_dav opens and closes it's internal prop
database for every resource found. I don't claim to know enough about the
internal workings of mod_dav to fix it, but thought I would post it here to
get gstein's attention when he gets back.
(I'll file an issue tomorrow as well).
For now: It is off to bed.
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: mod_dav/mod_dav_svn PROPFINDS.
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> See the end of:
>
> http://cvs.apache.org/viewcvs.cgi/apr-serf/docs/roadmap.txt?rev=HEAD&content-type=text/vnd.viewcvs-markup
>
> Personally, moving something like that to the APR site or docs and then
> referring to it would be Bestest(tm)
Thanks. I had some material already written, but this stuff
complemented it well. See commit 2487 to HACKING.
It would have to be expanded before being moved into APR, I think,
since Subversion's pool usage strategy is not necessarily recommended
for other projects (Apache comes to mind).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: mod_dav/mod_dav_svn PROPFINDS.
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Jul 11, 2002 at 05:09:44PM -0500, Karl Fogel wrote:
>...
> However, now that we do have a consistent method for using pools, at
> least the problem won't grow bigger. I think I'll add an item to
> HACKING, so this is easier for the next developer who joins...
See the end of:
http://cvs.apache.org/viewcvs.cgi/apr-serf/docs/roadmap.txt?rev=HEAD&content-type=text/vnd.viewcvs-markup
Personally, moving something like that to the APR site or docs and then
referring to it would be Bestest(tm)
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: mod_dav/mod_dav_svn PROPFINDS.
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> Very sloppy programming on my part. My only excuse is that mod_dav, its
> backend API, and the initial framework for mod_dav_svn were all done before
> I (we) really established a solid methodology for subpool usage. mod_dav is
> really crappy in terms of subpools (read: it doesn't use them at all).
Yeah, but don't beat yourself up too bad about it -- there's stuff
like this still going on all over the code base. Just now I was in
libsvn_wc/diff.c and noticed some more.
However, now that we do have a consistent method for using pools, at
least the problem won't grow bigger. I think I'll add an item to
HACKING, so this is easier for the next developer who joins...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: mod_dav/mod_dav_svn PROPFINDS.
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Jul 01, 2002 at 11:49:49AM -0700, Justin Erenkrantz wrote:
>...
> BTW, I *think* it is related to the fact that the dav_svn_do_walk
> function isn't creating a subpool.
>
> I'm specifically looking at subversion/mod_dav_svn.c:1963:
>
> /* fetch this collection's children */
> /* ### shall we worry about filling params->pool? */
> serr = svn_fs_dir_entries(&children, ctx->info.root.root,
> ctx->info.repos_path, params->pool);
>
> If we had svn blame, we could figure out who added the ### comment
> and I believe whomever said that is right.
You may not have svn blame, but you do have CVS:
http://subversion.tigris.org/source/browse/subversion/subversion/mod_dav_svn/repos.c.diff?r1=1.12&r2=1.13
IOW, I put that comment in on day one, when I wrote the function :-)
>...
> Thanks to Sander who pointed this out to me. I've been trying
> (unsuccessfully) to talk to him wrt to this after he pointed it
> out, so I'll post here in the hopes someone will jump on this.
>
> I'm not sure I have the time to play with this, but I'm willing to
> guess that this is the problem. -- justin
Thanks to all of you for looking into this and fixing it. Kudos!
Very sloppy programming on my part. My only excuse is that mod_dav, its
backend API, and the initial framework for mod_dav_svn were all done before
I (we) really established a solid methodology for subpool usage. mod_dav is
really crappy in terms of subpools (read: it doesn't use them at all).
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: mod_dav/mod_dav_svn PROPFINDS.
Posted by Justin Erenkrantz <je...@apache.org>.
On Mon, Jul 01, 2002 at 04:32:31PM -0400, Kevin Pilch-Bisson wrote:
> Unfortunately no. Sander and I tried just such a patch, and it helped, but
> did not solve the problem. That is, is reduced memory usage from 700MB to
> 400, but obviously that is still too high.
It probably means there are other places with poor pool usage.
Why not commit that patch and keep looking? -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: mod_dav/mod_dav_svn PROPFINDS.
Posted by Sander Striker <st...@apache.org>.
> From: Karl Fogel [mailto:kfogel@newton.ch.collab.net]
> Sent: 01 July 2002 22:39
[...]
> Hmmm. I would be surprised if that were the fix anyway. Usually a
> caller should not be creating a subpool for one function call. Maybe
> there are rare circumstances where it's justified -- but then, those
> are the sorts of circumstances where it would be even *more* justified
> for the called function to create and free its own subpool internally!
>
> Rather, callers should use clearable subpools for unbounded loops. I
> mean loops where the number of iterations is not constant, but is
> dependent on some run-time factor, such as the number of entries in a
> directory.
>
> So, I wonder if the real problem could be the loop immediately beneath
> the above-quoted code, the loop that begins:
>
> /* iterate over the children in this collection */
> for (hi = apr_hash_first(params->pool, children); hi; hi = apr_hash_next(hi))
> {
> ... do various stuff, including at least one more use of
> params->pool, hint hint :-) ...
> }
>
> Even if subpool'ing that loop doesn't solve the problem, it is
> probably still a good idea.
/me nods
This is what I quickly hack patched, and it does help, though not resolve the
entire problem.
> But Ben recalls Greg Stein saying that this might actually be a pool
> usage problem in mod_dav itself, so changes in mod_dav_svn may or may
> not be part of a solution. Dunno, back to you.
I guess it is 'take a close look at mod_dav' time...
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: mod_dav/mod_dav_svn PROPFINDS.
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> > I'm specifically looking at subversion/mod_dav_svn.c:1963:
> >
> > /* fetch this collection's children */
> > /* ### shall we worry about filling params->pool? */
> > serr = svn_fs_dir_entries(&children, ctx->info.root.root,
> > ctx->info.repos_path, params->pool);
Meant repos.c, by the way, not mod_dav_svn.c.
> > If we had svn blame, we could figure out who added the ### comment
> > and I believe whomever said that is right.
> >
> > Before calling this function, I think a subpool needs to be
> > created and then delete it as we exit this function. This will mean
> > that the pool will only grow to the depth of the recursive tree, but
> > no more than that rather than the unbounded growth we have now.
> >
> > [...]
>
> Unfortunately no. Sander and I tried just such a patch, and it helped, but
> did not solve the problem. That is, is reduced memory usage from 700MB to
> 400, but obviously that is still too high.
Hmmm. I would be surprised if that were the fix anyway. Usually a
caller should not be creating a subpool for one function call. Maybe
there are rare circumstances where it's justified -- but then, those
are the sorts of circumstances where it would be even *more* justified
for the called function to create and free its own subpool internally!
Rather, callers should use clearable subpools for unbounded loops. I
mean loops where the number of iterations is not constant, but is
dependent on some run-time factor, such as the number of entries in a
directory.
So, I wonder if the real problem could be the loop immediately beneath
the above-quoted code, the loop that begins:
/* iterate over the children in this collection */
for (hi = apr_hash_first(params->pool, children); hi; hi = apr_hash_next(hi))
{
... do various stuff, including at least one more use of
params->pool, hint hint :-) ...
}
Even if subpool'ing that loop doesn't solve the problem, it is
probably still a good idea.
But Ben recalls Greg Stein saying that this might actually be a pool
usage problem in mod_dav itself, so changes in mod_dav_svn may or may
not be part of a solution. Dunno, back to you.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: mod_dav/mod_dav_svn PROPFINDS.
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Mon, Jul 01, 2002 at 11:49:49AM -0700, Justin Erenkrantz wrote:
> BTW, I *think* it is related to the fact that the dav_svn_do_walk
> function isn't creating a subpool.
>
> I'm specifically looking at subversion/mod_dav_svn.c:1963:
>
> /* fetch this collection's children */
> /* ### shall we worry about filling params->pool? */
> serr = svn_fs_dir_entries(&children, ctx->info.root.root,
> ctx->info.repos_path, params->pool);
>
> If we had svn blame, we could figure out who added the ### comment
> and I believe whomever said that is right.
>
> Before calling this function, I think a subpool needs to be
> created and then delete it as we exit this function. This will mean
> that the pool will only grow to the depth of the recursive tree, but
> no more than that rather than the unbounded growth we have now.
>
> Thanks to Sander who pointed this out to me. I've been trying
> (unsuccessfully) to talk to him wrt to this after he pointed it
> out, so I'll post here in the hopes someone will jump on this.
>
> I'm not sure I have the time to play with this, but I'm willing to
> guess that this is the problem. -- justin
Unfortunately no. Sander and I tried just such a patch, and it helped, but
did not solve the problem. That is, is reduced memory usage from 700MB to
400, but obviously that is still too high.
>
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: mod_dav/mod_dav_svn PROPFINDS.
Posted by Justin Erenkrantz <je...@apache.org>.
On Sun, Jun 30, 2002 at 01:03:22AM -0400, Kevin Pilch-Bisson wrote:
> Hey,
>
> Trying to help someone on IRC today, I imported the linux kernel source into a
> repository. I then tried to check it out over dav and had problems. After
> watching for a while, I noticed that whenever the checkout started a new
> directory it would stop for quite a while, and the httpd memory usage would
> grow. I soon found out that my svn client would time out before getting a
> PROPFIND request back for the include/linux directory of the kernel sources.
>
> The reason for this is that my machine starts thrashing, because the httpd
> process trying to do the propfind grows huge (680MB before I killed it).(Note
> by contrast that opening the directory in w3m took about 2 seconds, and I
> didn't notice httpd's memory usage.
BTW, I *think* it is related to the fact that the dav_svn_do_walk
function isn't creating a subpool.
I'm specifically looking at subversion/mod_dav_svn.c:1963:
/* fetch this collection's children */
/* ### shall we worry about filling params->pool? */
serr = svn_fs_dir_entries(&children, ctx->info.root.root,
ctx->info.repos_path, params->pool);
If we had svn blame, we could figure out who added the ### comment
and I believe whomever said that is right.
Before calling this function, I think a subpool needs to be
created and then delete it as we exit this function. This will mean
that the pool will only grow to the depth of the recursive tree, but
no more than that rather than the unbounded growth we have now.
Thanks to Sander who pointed this out to me. I've been trying
(unsuccessfully) to talk to him wrt to this after he pointed it
out, so I'll post here in the hopes someone will jump on this.
I'm not sure I have the time to play with this, but I'm willing to
guess that this is the problem. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org