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 <cmpil...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
