[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Sub-pools for recursion

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2003-12-02 23:45:39 CET

Justin Erenkrantz wrote:
> --On Friday, November 28, 2003 2:35 PM +0000 Julian Foad
> <julianfoad@btopenworld.com> wrote:
>> I believe there are definite mis-uses of both "pool" and "sub-pool" in the
>> function. Here is an abbreviated and annotated version of the function; all
>> comments are mine:
>> copy_versioned_files (..., apr_pool_t *pool)
>> {
>> apr_pool_t *subpool = svn_pool_create (pool);
>> SVN_ERR (svn_wc_adm_probe_open (..., pool)); /* OK: needed
>> throughout */
>> err = svn_wc_entry (..., subpool); /* ### Wrong: needed throughout */
> entry is only used once at first - the subpool is indeed safe. Then,

Yes, you are right: that use of "subpool" was safe. Nevertheless, now that we have decided that the sub-pool should be for iteration, I think it is poor style to use it also for this unrelated purpose. Doing so was more space-efficient (because this allocation was cleared at the beginning of iteration), but the correct use of pools is not simple and I don't think this small optimisation justifies the extra complexity.

> look at when we obtain entry in the svn_node_file path with subpool again.
>> if (entry)
>> {
>> SVN_ERR (svn_io_stat (&finfo, ..., subpool)); /* Safe (used only
>> once), but might as well go in "pool" */
> svn_io_stat is a ridiculous memory hog. It needs to be in the subpool.

I'm not sure what you mean here. As far as I can see, svn_io_stat allocates in the order of 100 bytes. This call is not within the iteration; it is only done once within the body of this function, so once per level of recursion overall.

The pool "pool" passed into copy_versioned_files is intended to be used for allocating (one-off) temporary stuff. Therefore this should go in "pool". If the caller is going to call multiple times, then the caller should provide a sub-pool (and this function, when calling itself in an iteration, does so). I believe this is standard practice and not a problem.

>> SVN_ERR (svn_subst_build_keywords
>> (...,
>> apr_psprintf (pool, ...), /* Safe but
>> should go
>> in "subpool" */
> Agreed. That printf is very recent (I had to update my WC to get it),
> so that'll probably explain why it has improper memory usage.

OK, I'll fix that.

> I'm also concerned that adm_access could be used after it is closed in
> copy_versioned_files. Philip? -- justin

Yes. I'll fix that too.

Patch attached for review.

- Julian

Fix access baton lifetime and use pools more consistently.

* subversion/libsvn_client/export.c
  (copy_versioned_files): Use the sub-pool only for iteration, and the main
    pool for all other allocations.
    Rename "subpool" to "iterpool" to make this intention clear.
    Do not close the adm_access baton until we have finished using it.
    Eliminate the wrong error handling that would have occurred if
    svn_wc_adm_close failed.

Index: subversion/libsvn_client/export.c
--- subversion/libsvn_client/export.c (revision 7899)
+++ subversion/libsvn_client/export.c (working copy)
@@ -49,16 +49,13 @@
                       svn_client_ctx_t *ctx,
                       apr_pool_t *pool)
- apr_pool_t *subpool = svn_pool_create (pool);
- apr_hash_t *dirents;
   svn_wc_adm_access_t *adm_access;
   const svn_wc_entry_t *entry;
   svn_error_t *err;
   SVN_ERR (svn_wc_adm_probe_open (&adm_access, NULL, from, FALSE,
                                   FALSE, pool));
- err = svn_wc_entry (&entry, from, adm_access, FALSE, subpool);
- SVN_ERR (svn_wc_adm_close (adm_access));
+ err = svn_wc_entry (&entry, from, adm_access, FALSE, pool);
   if (err)
       if (err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
@@ -70,15 +67,17 @@
   /* We don't want to copy some random non-versioned directory. */
   if (entry)
+ apr_pool_t *iterpool = svn_pool_create (pool);
+ apr_hash_t *dirents;
       apr_hash_index_t *hi;
       apr_finfo_t finfo;
- SVN_ERR (svn_io_stat (&finfo, from, APR_FINFO_PROT, subpool));
+ SVN_ERR (svn_io_stat (&finfo, from, APR_FINFO_PROT, pool));
       /* Try to make the new directory. If this fails because the
          directory already exists, check our FORCE flag to see if we
          care. */
- err = svn_io_dir_make (to, finfo.protection, subpool);
+ err = svn_io_dir_make (to, finfo.protection, pool);
       if (err)
           if (! APR_STATUS_IS_EEXIST (err->apr_err))
@@ -119,20 +118,20 @@
- const char *new_from = svn_path_join (from, key, subpool);
- const char *new_to = svn_path_join (to, key, subpool);
+ const char *new_from = svn_path_join (from, key, iterpool);
+ const char *new_to = svn_path_join (to, key, iterpool);
                   SVN_ERR (copy_versioned_files (new_from, new_to, revision,
- force, ctx, subpool));
+ force, ctx, iterpool));
           else if (*type == svn_node_file)
- const char *copy_from = svn_path_join (from, item, subpool);
- const char *copy_to = svn_path_join (to, item, subpool);
+ const char *copy_from = svn_path_join (from, item, iterpool);
+ const char *copy_to = svn_path_join (to, item, iterpool);
               err = svn_wc_entry (&entry, copy_from, adm_access, FALSE,
- subpool);
+ iterpool);
               if (err)
@@ -158,9 +157,9 @@
                   if (revision->kind != svn_opt_revision_working)
                       SVN_ERR (svn_wc_get_pristine_copy_path
- (copy_from, &base, subpool));
+ (copy_from, &base, iterpool));
                       SVN_ERR (svn_wc_get_prop_diffs
- (NULL, &props, copy_from, adm_access, subpool));
+ (NULL, &props, copy_from, adm_access, iterpool));
@@ -168,8 +167,8 @@
                       base = copy_from;
                       SVN_ERR (svn_wc_prop_list (&props, copy_from,
- adm_access, subpool));
- SVN_ERR (svn_wc_status (&status, copy_from, adm_access, subpool));
+ adm_access, iterpool));
+ SVN_ERR (svn_wc_status (&status, copy_from, adm_access, iterpool));
                       if (status->text_status != svn_wc_status_normal)
                         local_mod = TRUE;
@@ -189,7 +188,7 @@
                   if (local_mod)
                     /* Use the modified time from the working copy if the file */
- SVN_ERR (svn_io_file_affected_time (&time, copy_from, subpool));
+ SVN_ERR (svn_io_file_affected_time (&time, copy_from, iterpool));
                     time = entry->cmt_date;
@@ -215,32 +214,32 @@
                       SVN_ERR (svn_subst_build_keywords
                                (&kw, keywords->data,
- apr_psprintf (pool,
+ apr_psprintf (iterpool,
- subpool));
+ iterpool));
                   SVN_ERR (svn_subst_copy_and_translate (base, copy_to,
                                                          eol, FALSE,
                                                          &kw, TRUE,
- subpool));
+ iterpool));
                   if (executable)
- SVN_ERR (svn_io_set_file_executable (copy_to, TRUE, FALSE, subpool));
+ SVN_ERR (svn_io_set_file_executable (copy_to, TRUE, FALSE, iterpool));
- SVN_ERR (svn_io_set_file_affected_time (time, copy_to, subpool));
+ SVN_ERR (svn_io_set_file_affected_time (time, copy_to, iterpool));
- svn_pool_clear (subpool);
+ svn_pool_clear (iterpool);
+ svn_pool_destroy (iterpool);
- svn_pool_destroy (subpool);
+ SVN_ERR (svn_wc_adm_close (adm_access));
   return SVN_NO_ERROR;

To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Dec 2 23:41:56 2003

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.