Re: Sub-pools for recursion

[Julian Foad <ju...@btopenworld.com>]

Justin Erenkrantz wrote:
> --On Friday, November 28, 2003 2:35 PM +0000 Julian Foad 
> <ju...@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