You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2004/03/15 17:32:39 UTC
Re: svn commit: r9032 - trunk/subversion/libsvn_wc
philip@tigris.org writes:
> Log:
> Fix part of issue 1635. Control client memory use when deleting
> lots of targets.
I might be missing something, but it looks like this commit controls
pool usage by creating a subpool at the top of a do_entry_deletion(),
then destroying it at the bottom. That is, it's not really an
iteration pool, it's a function-body pool.
Usually, we would create an iteration subpool in the caller(s), and
simply pass the subpool to do_entry_deletion(). In this case the
caller is ultimately an editor drive, but still, wouldn't the same
principle apply?
-Karl
> * subversion/libsvn_wc/update_editor.c (do_entry_deletion): Use a
> subpool.
>
>
> Modified: trunk/subversion/libsvn_wc/update_editor.c
> ==============================================================================
> --- trunk/subversion/libsvn_wc/update_editor.c (original)
> +++ trunk/subversion/libsvn_wc/update_editor.c Sat Mar 13 15:51:00 2004
> @@ -798,15 +798,16 @@
> svn_wc_adm_access_t *adm_access;
> svn_node_kind_t kind;
> const char *logfile_path;
> - const char *full_path = svn_path_join (eb->anchor, path, pool);
> - svn_stringbuf_t *log_item = svn_stringbuf_create ("", pool);
> + apr_pool_t *subpool = svn_pool_create (pool);
> + const char *full_path = svn_path_join (eb->anchor, path, subpool);
> + svn_stringbuf_t *log_item = svn_stringbuf_create ("", subpool);
>
> - SVN_ERR (svn_io_check_path (full_path, &kind, pool));
> + SVN_ERR (svn_io_check_path (full_path, &kind, subpool));
>
> SVN_ERR (svn_wc_adm_retrieve (&adm_access, eb->adm_access,
> - parent_path, pool));
> + parent_path, subpool));
>
> - logfile_path = svn_wc__adm_path (parent_path, FALSE, pool,
> + logfile_path = svn_wc__adm_path (parent_path, FALSE, subpool,
> SVN_WC__ADM_LOG, NULL);
>
> /* If trying to delete a locally-modified file, throw an 'obstructed
> @@ -815,9 +816,9 @@
> {
> svn_boolean_t tmodified_p, pmodified_p;
> SVN_ERR (svn_wc_text_modified_p (&tmodified_p, full_path, FALSE,
> - adm_access, pool));
> + adm_access, subpool));
> SVN_ERR (svn_wc_props_modified_p (&pmodified_p, full_path,
> - adm_access, pool));
> + adm_access, subpool));
>
> if (tmodified_p || pmodified_p)
> return svn_error_createf
> @@ -830,7 +831,7 @@
> parent_path,
> SVN_WC__ADM_LOG,
> (APR_WRITE | APR_CREATE), /* not excl */
> - pool));
> + subpool));
>
> /* Here's the deal: in the new editor interface, PATH is a full path
> below the editor's anchor, and parent_path is the parent directory.
> @@ -838,9 +839,9 @@
> log commands talk *only* about paths relative (and below)
> parent_path, i.e. where the log is being executed. */
>
> - base_name = svn_path_basename (path, pool);
> + base_name = svn_path_basename (path, subpool);
> svn_xml_make_open_tag (&log_item,
> - pool,
> + subpool,
> svn_xml_self_closing,
> SVN_WC__LOG_DELETE_ENTRY,
> SVN_WC__LOG_ATTR_NAME,
> @@ -852,10 +853,10 @@
> accurate reports about itself in the future. */
> if (strcmp (path, eb->target) == 0)
> {
> - tgt_rev_str = apr_psprintf (pool, "%" SVN_REVNUM_T_FMT,
> + tgt_rev_str = apr_psprintf (subpool, "%" SVN_REVNUM_T_FMT,
> *(eb->target_revision));
>
> - svn_xml_make_open_tag (&log_item, pool, svn_xml_self_closing,
> + svn_xml_make_open_tag (&log_item, subpool, svn_xml_self_closing,
> SVN_WC__LOG_MODIFY_ENTRY,
> SVN_WC__LOG_ATTR_NAME,
> path,
> @@ -873,15 +874,15 @@
> }
>
> SVN_ERR_W (svn_io_file_write_full (log_fp, log_item->data,
> - log_item->len, NULL, pool),
> - apr_psprintf (pool,
> + log_item->len, NULL, subpool),
> + apr_psprintf (subpool,
> "Error writing log file for '%s'", parent_path));
>
> SVN_ERR (svn_wc__close_adm_file (log_fp,
> parent_path,
> SVN_WC__ADM_LOG,
> TRUE, /* sync */
> - pool));
> + subpool));
>
> if (eb->switch_url)
> {
> @@ -906,7 +907,7 @@
>
> SVN_ERR (svn_wc_adm_retrieve
> (&child_access, eb->adm_access,
> - full_path, pool));
> + full_path, subpool));
>
> SVN_ERR (leftmod_error_chain
> (svn_wc_remove_from_revision_control
> @@ -916,13 +917,13 @@
> TRUE, /* instant error */
> eb->cancel_func,
> eb->cancel_baton,
> - pool),
> - logfile_path, parent_path, pool));
> + subpool),
> + logfile_path, parent_path, subpool));
> }
> }
>
> - SVN_ERR (leftmod_error_chain (svn_wc__run_log (adm_access, NULL, pool),
> - logfile_path, parent_path, pool));
> + SVN_ERR (leftmod_error_chain (svn_wc__run_log (adm_access, NULL, subpool),
> + logfile_path, parent_path, subpool));
>
> /* The passed-in `path' is relative to the anchor of the edit, so if
> * the operation was invoked on something other than ".", then
> @@ -932,7 +933,7 @@
> */
> if (eb->notify_func)
> (*eb->notify_func) (eb->notify_baton,
> - svn_path_join (parent_path, base_name, pool),
> + svn_path_join (parent_path, base_name, subpool),
> svn_wc_notify_update_delete,
> svn_node_unknown,
> NULL,
> @@ -940,6 +941,7 @@
> svn_wc_notify_state_unknown,
> SVN_INVALID_REVNUM);
>
> + svn_pool_destroy (subpool);
> return SVN_NO_ERROR;
> }
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r9032 - trunk/subversion/libsvn_wc
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2004-03-15 at 12:32, kfogel@collab.net wrote:
> Usually, we would create an iteration subpool in the caller(s), and
> simply pass the subpool to do_entry_deletion(). In this case the
> caller is ultimately an editor drive, but still, wouldn't the same
> principle apply?
Agreed; changing the drivers to use an iteration subpool for deleting
entries would probably be a better fix.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r9032 - trunk/subversion/libsvn_wc
Posted by kf...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> You are correct. It turns out that the server fixes (r9033/r9044)
> already solve the problem for ra_svn, and ra_local was never
> affected. That only leaves ra_dav, how about this
>
> Index: subversion/libsvn_ra_dav/fetch.c
> ===================================================================
> --- subversion/libsvn_ra_dav/fetch.c (revision 9049)
> +++ subversion/libsvn_ra_dav/fetch.c (working copy)
> @@ -1788,7 +1788,7 @@
> svn_stringbuf_set(rb->namestr, name);
>
> parent_dir = &TOP_DIR(rb);
> - subpool = parent_dir->pool;
> + subpool = svn_pool_create (parent_dir->pool);
>
> pathbuf = svn_stringbuf_dup(parent_dir->pathbuf, subpool);
> svn_path_add_component(pathbuf, rb->namestr->data);
> @@ -1797,6 +1797,7 @@
> SVN_INVALID_REVNUM,
> TOP_DIR(rb).baton,
> subpool) );
> + svn_pool_destroy (subpool);
> break;
>
> default:
All of a sudden, I feel weird about this change.
libsvn_ra_dav/fetch.c:start_element() passes different pools to
different editor functions. Sometimes it uses rb->ras->pool,
sometimes it uses parent_dir->pool. And in some case, it creates a
subpool of parent_dir->pool and 'push_dir()'s that subpool onto the
directory stack.
(It's confusing enough that in the current code, 'subpool' is a
karmically inoperative intermediary in the delete_entry case. Why
aren't we just passing parent_dir->pool directly? Well, maybe I just
wanted an excuse to use the phrase "karmically inoperative".)
Anyway, now here we are, about to use a subpool in a totally new way
in this function: it will be passed to an editor function (not in a
loop), then be immediately destroyed. Anyone looking at that would
wonder, what does this caller know about the editor function's
internal implementation that makes a subpool desirable? And *why*
does the caller know it?
In other words, neither subpool-in-caller nor subpool-in-callee is in
harmony with our usual subpool practice, in this case. The problem is
that the "loop" we're in is an XML parsing loop, and there isn't any
way to get the parser to spin off a subpool for each element handler
invocation, and destroy that subpool at the right time. The pools are
all in our batons.
Oh, and even if we *could* get a dedicated subpool passed into the
function, we wouldn't always want to use it, because we need certain
objects created in the function call to live longer than the call...
Okay, that's a lot of thinking out loud. Gist of it is: a comment at
the subpool creation explaining why we're doing things this way might
help reduce the confusion for future visitors.
Your call. I think the underlying issue here is that some pool usage
ugliness is forced on us by the fact that our code is sometimes driven
by third-party libraries.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r9032 - trunk/subversion/libsvn_wc
Posted by Philip Martin <ph...@codematters.co.uk>.
kfogel@collab.net writes:
> philip@tigris.org writes:
>> Log:
>> Fix part of issue 1635. Control client memory use when deleting
>> lots of targets.
>
> I might be missing something, but it looks like this commit controls
> pool usage by creating a subpool at the top of a do_entry_deletion(),
> then destroying it at the bottom. That is, it's not really an
> iteration pool, it's a function-body pool.
>
> Usually, we would create an iteration subpool in the caller(s), and
> simply pass the subpool to do_entry_deletion(). In this case the
> caller is ultimately an editor drive, but still, wouldn't the same
> principle apply?
You are correct. It turns out that the server fixes (r9033/r9044)
already solve the problem for ra_svn, and ra_local was never
affected. That only leaves ra_dav, how about this
Index: subversion/libsvn_ra_dav/fetch.c
===================================================================
--- subversion/libsvn_ra_dav/fetch.c (revision 9049)
+++ subversion/libsvn_ra_dav/fetch.c (working copy)
@@ -1788,7 +1788,7 @@
svn_stringbuf_set(rb->namestr, name);
parent_dir = &TOP_DIR(rb);
- subpool = parent_dir->pool;
+ subpool = svn_pool_create (parent_dir->pool);
pathbuf = svn_stringbuf_dup(parent_dir->pathbuf, subpool);
svn_path_add_component(pathbuf, rb->namestr->data);
@@ -1797,6 +1797,7 @@
SVN_INVALID_REVNUM,
TOP_DIR(rb).baton,
subpool) );
+ svn_pool_destroy (subpool);
break;
default:
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org