You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@collab.net> on 2012/08/10 08:20:40 UTC
Re: [PATCH] Make ra_serf be more aggressive about closing directories
in update
Justin, you appear to have resurfaced (to a degree, at least). Did you ever
complete this work?
On 06/18/2012 02:18 AM, Justin Erenkrantz wrote:
> One issue that I've come across in my recent analysis is that ra_serf
> isn't always aggressive in closing directories when doing an update.
> Specifically, when the directory propfind isn't received by the time
> all of the files are processed, ra_serf won't look at the directory
> handle again until everything is complete. (It only evaluates when
> closing the files.) When doing a large checkout (like tags/), this
> can be a bit problematic as we really need to be aggressive in
> reclaiming all of the pools to keep our memory usage down as much as
> possible.
>
> This patch is unfinished...it solves the main issue, but there is some
> corruption in the active_dir_propfinds (which I sort of tried to
> address by using serf's allocator; which isn't a normal paradigm for
> Subversion). When I get back home today, I need to concentrate on my
> pending cross-country move and starting my new position in NYC. So,
> it may be a few weeks before I get any real chance to look at this
> again.
>
> In the meantime, I know that Greg is planning to rewrite the XML
> parsing in update.c to fit the new parsing paradigm that has been
> introduced into other parts of ra_serf as well as getting the Propfind
> depth:1 in. My guess it that after the parsing rewrite and depth 1 is
> complete, this issue may fall by the wayside...or not.
>
> Hence, I'll just throw the current patch on the list for posterity.
> When I come up for air again, I'll try to evaluate if it is still
> needed. FWIW, Instruments on OS X is awesome. -- justin
>
> Index: subversion/libsvn_ra_serf/update.c
> ===================================================================
> --- subversion/libsvn_ra_serf/update.c (revision 1351209)
> +++ subversion/libsvn_ra_serf/update.c (working copy)
> @@ -48,6 +48,8 @@
> #include "ra_serf.h"
> #include "../libsvn_ra/ra_loader.h"
>
> +#include "serf_bucket_util.h"
> +
>
> /*
> * This enum represents the current state of our XML parsing for a REPORT.
> @@ -292,6 +294,7 @@ typedef struct report_fetch_t {
> */
> struct report_context_t {
> apr_pool_t *pool;
> + serf_bucket_alloc_t *allocator;
>
> svn_ra_serf__session_t *sess;
> svn_ra_serf__connection_t *conn;
> @@ -348,7 +351,11 @@ struct report_context_t {
>
> /* completed PROPFIND requests (contains svn_ra_serf__handler_t) */
> svn_ra_serf__list_t *done_propfinds;
> + svn_ra_serf__list_t *done_dir_propfinds;
>
> + /* list of outstanding prop changes (contains report_dir_t) */
> + svn_ra_serf__list_t *active_dir_propfinds;
> +
> /* list of files that only have prop changes (contains report_info_t) */
> svn_ra_serf__list_t *file_propchanges_only;
>
> @@ -654,6 +661,7 @@ close_dir(report_dir_t *dir)
> }
>
> svn_pool_destroy(dir->dir_baton_pool);
> + printf("destroy: %p (%s)\n", dir->pool, dir->name);
> svn_pool_destroy(dir->pool);
>
> return SVN_NO_ERROR;
> @@ -1888,6 +1896,7 @@ end_report(svn_ra_serf__xml_parser_t *parser,
> */
> if (!SVN_IS_VALID_REVNUM(info->dir->base_rev) || info->dir->fetch_props)
> {
> + svn_ra_serf__list_t *list_item;
> /* Unconditionally set fetch_props now. */
> info->dir->fetch_props = TRUE;
>
> @@ -1897,7 +1906,7 @@ end_report(svn_ra_serf__xml_parser_t *parser,
> info->dir->url,
> ctx->target_rev, "0",
> all_props,
> - &ctx->done_propfinds,
> + &ctx->done_dir_propfinds,
> info->dir->pool));
> SVN_ERR_ASSERT(info->dir->propfind_handler);
>
> @@ -1906,6 +1915,11 @@ end_report(svn_ra_serf__xml_parser_t *parser,
>
> ctx->active_propfinds++;
>
> + list_item = serf_bucket_mem_alloc(ctx->allocator,
> sizeof(*list_item));
> + list_item->data = info->dir;
> + list_item->next = ctx->active_dir_propfinds;
> + ctx->active_dir_propfinds = list_item;
> +
> if (ctx->active_fetches + ctx->active_propfinds
> > REQUEST_COUNT_TO_PAUSE)
> ctx->parser_ctx->paused = TRUE;
> @@ -2525,12 +2539,77 @@ finish_report(void *report_baton,
> }
> report->done_propfinds = NULL;
>
> + done_list = report->done_dir_propfinds;
> + while (done_list)
> + {
> + svn_pool_clear(iterpool_inner);
> +
> + report->active_propfinds--;
> +
> + if (report->active_dir_propfinds)
> + {
> + svn_ra_serf__list_t *cur, *prev;
> +
> + cur = report->active_dir_propfinds;
> + prev = NULL;
> +
> + while (cur)
> + {
> + report_dir_t *item = cur->data;
> +
> + if (item->propfind_handler == done_list->data)
> + {
> + break;
> + }
> +
> + prev = cur;
> + cur = cur->next;
> + }
> + if (cur)
> + {
> + report_dir_t *cur_dir = cur->data;
> +
> + while (cur_dir && !cur_dir->ref_count && cur_dir->tag_closed
> + && (!cur_dir->fetch_props
> + || cur_dir->propfind_handler->done))
> + {
> + printf("clear: %p : %s\n", cur_dir, cur_dir->name);
> + report_dir_t *parent = cur_dir->parent_dir;
> +
> + SVN_ERR(close_dir(cur_dir));
> + if (parent)
> + {
> + parent->ref_count--;
> + }
> + else
> + {
> + closed_root = TRUE;
> + }
> + cur_dir = parent;
> + }
> +
> + if (!prev)
> + {
> + report->active_dir_propfinds = cur->next;
> + }
> + else
> + {
> + prev->next = cur->next;
> + }
> + serf_bucket_mem_free(report->allocator, cur);
> + }
> + }
> +
> + done_list = done_list->next;
> + }
> + report->done_dir_propfinds = NULL;
> +
> /* prune our fetches list if they are done. */
> done_list = report->done_fetches;
> while (done_list)
> {
> report_fetch_t *done_fetch = done_list->data;
> - report_dir_t *cur_dir;
> + report_dir_t *cur_dir, *t_d;
>
> /* decrease our parent's directory refcount. */
> cur_dir = done_fetch->info->dir;
> @@ -2549,6 +2628,17 @@ finish_report(void *report_baton,
> * we've already completed the propfind
> * then, we know it's time for us to close this directory.
> */
> + t_d = cur_dir;
> + while (t_d) {
> + printf("%p : %s %ld ", t_d, t_d->name, t_d->ref_count);
> + if (t_d->ref_count) {
> + printf("\n");
> + } else {
> + printf("-###- %d %d %p %d\n", t_d->tag_closed,
> t_d->fetch_props, t_d->propfind_handler, t_d->propfind_handler ?
> t_d->propfind_handler->done : -1);
> + }
> + t_d = t_d->parent_dir;
> + }
> +
> while (cur_dir && !cur_dir->ref_count && cur_dir->tag_closed
> && (!cur_dir->fetch_props
> || cur_dir->propfind_handler->done))
> @@ -2674,6 +2764,7 @@ make_update_reporter(svn_ra_session_t *ra_session,
>
> report = apr_pcalloc(result_pool, sizeof(*report));
> report->pool = result_pool;
> + report->allocator = serf_bucket_allocator_create(report->pool, NULL, NULL);
> report->sess = sess;
> report->conn = report->sess->conns[0];
> report->target_rev = revision;
>
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
RE: [PATCH] Make ra_serf be more aggressive about closing directories in update
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: vrijdag 31 augustus 2012 14:30
> To: C. Michael Pilato
> Cc: Justin Erenkrantz; Subversion Development
> Subject: Re: [PATCH] Make ra_serf be more aggressive about closing
> directories in update
>
> On Fri, Aug 24, 2012 at 5:56 PM, C. Michael Pilato <cm...@collab.net>
> wrote:
> > On 08/10/2012 06:20 AM, Justin Erenkrantz wrote:
> >> FWIW, last week, I went down the path of trying to optimize the 'ls'
> >> client path - which issues two OPTIONS requests...which is kinda
> >> silly. but to remove it, requires that the RA open call also return
> >> the youngest revision. I rev'd all of the RA layers, but then got
> >> stuck in a maze of twisty calls inside libsvn_client to try and use
> >> that revnum to skip the later call which invokes OPTIONS again...I
> >> should probably post that incomplete patch before moving on to
> >> something else. =)
> >
> > If it makes you feel better, I've been down this road before, too. :-)
> >
> > One thing that occurs to me as possible solution is to add a flag to
> > svn_ra_openX() which instructs the RA layer to immediately negotiate the
> > current youngest rev and then freeze that snapshot of the repository for
> the
> > duration of the RA session's lifetime. The client layer alone knows whether
> > the high-level operation it is performing would be effected by additional
> > commits to the repository. So for read-only operations, the clients would
> > pass this flag to the RA open stuffs, and the RA layer would cache the
> > youngest-rev. Future calls to svn_ra_get_latest_revnum() on that session
> > would always return the cached value. Other RA calls that allow you to use
> > SVN_INVALID_REVNUM to mean "HEAD" would also use the cached
> youngest-rev.
> >
> > Is that crazy?
> >
> > My one concern is that it might negatively effect code clarity. If I see
> > svn_ra_get_latest_revnum(ra_session, &youngest, pool) in the client
> code, I
> > can't immediately tell if that function call will get the *realtime*
> > youngest revision or the frozen snapshot revision. I'd need to find the
> > ra-open call to see if that special flag was provided.
> >
> >
> > -- C-Mike
> >
> > PS: Going really nuts, we *could* allow the RA session to "peg" not merely
> > the current HEAD, but any arbitrary revision as the youngest revision the
> RA
> > session will be allowed to recognize. But I can't quickly come up with a
> > good reason for doing that.
> >
> For me it looks like abstraction leakage. Better solution would be
> create new object like svn_client_ra_session_t() and store
> cached/negotiate latest revision here, while extend
> svn_ra_session_open to return youngest revision if requested.
We could just rev the function that retrieves the last revision to allow telling it that returning a cached value is ok. (Or add a different function for the potentially cached value)
Creating the sub-ra_sessions is probably too much for this optimization, but it could be useful for other things too.
Bert
Re: [PATCH] Make ra_serf be more aggressive about closing directories
in update
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Fri, Aug 24, 2012 at 5:56 PM, C. Michael Pilato <cm...@collab.net> wrote:
> On 08/10/2012 06:20 AM, Justin Erenkrantz wrote:
>> FWIW, last week, I went down the path of trying to optimize the 'ls'
>> client path - which issues two OPTIONS requests...which is kinda
>> silly. but to remove it, requires that the RA open call also return
>> the youngest revision. I rev'd all of the RA layers, but then got
>> stuck in a maze of twisty calls inside libsvn_client to try and use
>> that revnum to skip the later call which invokes OPTIONS again...I
>> should probably post that incomplete patch before moving on to
>> something else. =)
>
> If it makes you feel better, I've been down this road before, too. :-)
>
> One thing that occurs to me as possible solution is to add a flag to
> svn_ra_openX() which instructs the RA layer to immediately negotiate the
> current youngest rev and then freeze that snapshot of the repository for the
> duration of the RA session's lifetime. The client layer alone knows whether
> the high-level operation it is performing would be effected by additional
> commits to the repository. So for read-only operations, the clients would
> pass this flag to the RA open stuffs, and the RA layer would cache the
> youngest-rev. Future calls to svn_ra_get_latest_revnum() on that session
> would always return the cached value. Other RA calls that allow you to use
> SVN_INVALID_REVNUM to mean "HEAD" would also use the cached youngest-rev.
>
> Is that crazy?
>
> My one concern is that it might negatively effect code clarity. If I see
> svn_ra_get_latest_revnum(ra_session, &youngest, pool) in the client code, I
> can't immediately tell if that function call will get the *realtime*
> youngest revision or the frozen snapshot revision. I'd need to find the
> ra-open call to see if that special flag was provided.
>
>
> -- C-Mike
>
> PS: Going really nuts, we *could* allow the RA session to "peg" not merely
> the current HEAD, but any arbitrary revision as the youngest revision the RA
> session will be allowed to recognize. But I can't quickly come up with a
> good reason for doing that.
>
For me it looks like abstraction leakage. Better solution would be
create new object like svn_client_ra_session_t() and store
cached/negotiate latest revision here, while extend
svn_ra_session_open to return youngest revision if requested.
--
Ivan Zhakov
Re: [PATCH] Make ra_serf be more aggressive about closing directories
in update
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Fri, Aug 24, 2012 at 9:56 AM, C. Michael Pilato <cm...@collab.net> wrote:
> If it makes you feel better, I've been down this road before, too. :-)
*grin*
> One thing that occurs to me as possible solution is to add a flag to
> svn_ra_openX() which instructs the RA layer to immediately negotiate the
> current youngest rev and then freeze that snapshot of the repository for the
> duration of the RA session's lifetime. The client layer alone knows whether
> the high-level operation it is performing would be effected by additional
> commits to the repository. So for read-only operations, the clients would
> pass this flag to the RA open stuffs, and the RA layer would cache the
> youngest-rev. Future calls to svn_ra_get_latest_revnum() on that session
> would always return the cached value. Other RA calls that allow you to use
> SVN_INVALID_REVNUM to mean "HEAD" would also use the cached youngest-rev.
>
> Is that crazy?
I didn't go so far as to freezing the session, but it's not completely
bonkers. =) I thought about it for a half-a-second and thought that
pegging the youngest rev to the session might have some awkward
side-effects - mainly due to a single session that does a youngest,
sends a commit, and then wants the youngest rev. That could be
badness...though I guess in theory the client side could invalidate
its cache when it does a commit...but...awkwardness lies behind that
door...
> My one concern is that it might negatively effect code clarity. If I see
> svn_ra_get_latest_revnum(ra_session, &youngest, pool) in the client code, I
> can't immediately tell if that function call will get the *realtime*
> youngest revision or the frozen snapshot revision. I'd need to find the
> ra-open call to see if that special flag was provided.
Yup. That's why it's only moderately bonkers.
I think if we were to really do this, we should have a proper API and
not slipstream it into the open call. (Perhaps get_latest_revnum
could grow a parameter specifying if its okay to utilize a cached
value??)
> PS: Going really nuts, we *could* allow the RA session to "peg" not merely
> the current HEAD, but any arbitrary revision as the youngest revision the RA
> session will be allowed to recognize. But I can't quickly come up with a
> good reason for doing that.
As Daniel pointed out, that capability would help some awkward
scenarios that would likely simplify some of our automated test
invocations. -- justin
Re: [PATCH] Make ra_serf be more aggressive about closing
directories in update
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Fri, Aug 24, 2012 at 09:56:25 -0400:
> PS: Going really nuts, we *could* allow the RA session to "peg" not merely
> the current HEAD, but any arbitrary revision as the youngest revision the RA
> session will be allowed to recognize. But I can't quickly come up with a
> good reason for doing that.
It would make Paul's reproduction recipes easier to read:
% export SVN_PRETEND_HEAD_IS=1954324
% svn co $URL_TRUNK
% svn merge $URL_FEATURE_BRANCH trunk
instead of having to peg the 'svn co' and twist the 'svn merge'
invocations.
Re: [PATCH] Make ra_serf be more aggressive about closing directories
in update
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/10/2012 06:20 AM, Justin Erenkrantz wrote:
> FWIW, last week, I went down the path of trying to optimize the 'ls'
> client path - which issues two OPTIONS requests...which is kinda
> silly. but to remove it, requires that the RA open call also return
> the youngest revision. I rev'd all of the RA layers, but then got
> stuck in a maze of twisty calls inside libsvn_client to try and use
> that revnum to skip the later call which invokes OPTIONS again...I
> should probably post that incomplete patch before moving on to
> something else. =)
If it makes you feel better, I've been down this road before, too. :-)
One thing that occurs to me as possible solution is to add a flag to
svn_ra_openX() which instructs the RA layer to immediately negotiate the
current youngest rev and then freeze that snapshot of the repository for the
duration of the RA session's lifetime. The client layer alone knows whether
the high-level operation it is performing would be effected by additional
commits to the repository. So for read-only operations, the clients would
pass this flag to the RA open stuffs, and the RA layer would cache the
youngest-rev. Future calls to svn_ra_get_latest_revnum() on that session
would always return the cached value. Other RA calls that allow you to use
SVN_INVALID_REVNUM to mean "HEAD" would also use the cached youngest-rev.
Is that crazy?
My one concern is that it might negatively effect code clarity. If I see
svn_ra_get_latest_revnum(ra_session, &youngest, pool) in the client code, I
can't immediately tell if that function call will get the *realtime*
youngest revision or the frozen snapshot revision. I'd need to find the
ra-open call to see if that special flag was provided.
-- C-Mike
PS: Going really nuts, we *could* allow the RA session to "peg" not merely
the current HEAD, but any arbitrary revision as the youngest revision the RA
session will be allowed to recognize. But I can't quickly come up with a
good reason for doing that.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: [PATCH] Make ra_serf be more aggressive about closing directories
in update
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Fri, Aug 10, 2012 at 2:20 AM, C. Michael Pilato <cm...@collab.net> wrote:
> Justin, you appear to have resurfaced (to a degree, at least). Did you ever
> complete this work?
No.
AFAICT, the rewrite of the XML parsing used in update.c and the Depth:
1 PROPFIND hasn't been completed either. I think it probably makes
sense to complete that and then evaluate whether we still need to
tackle this memory issue.
FWIW, last week, I went down the path of trying to optimize the 'ls'
client path - which issues two OPTIONS requests...which is kinda
silly, but to remove it, requires that the RA open call also return
the youngest revision. I rev'd all of the RA layers, but then got
stuck in a maze of twisty calls inside libsvn_client to try and use
that revnum to skip the later call which invokes OPTIONS again...I
should probably post that incomplete patch before moving on to
something else. =)
HTH. -- justin