You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2005/05/11 14:29:14 UTC
SVN_ERR bypassing "goto cleanup" [was: svn commit: r14643 - ...]
[Making this thread more visible and following up.]
Philip Martin wrote:
> I decided to have a look at our existing uses of "goto cleanup", just
> to see how many had SVN_ERRs that bypassed the cleanup.
>
> These look like real errors:
>
> - libsvn_wc/adm_crawler.c:svn_wc_crawl_revisions2
> fails to call reporter->abort_report
>
> - libsvn_ra_dav/util.c:parsed_request
> fails to destroy neon structures
>
> - libsvn_fs_base/fs.c:base_create
> - libsvn_fs_base/fs.c:base_open
> fails to call cleanup_fs
>
> - libsvn_client/commit.c:svn_client_commit
> fails to call editor->abort_edit
Philip fixed those in r14667.
> Leaving a subpool isn't really a problem, so these are not bugs:
>
> - libsvn_wc/questions.c:svn_wc_text_modified_p
> - libsvn_wc/props.c:svn_wc_props_modified_p
> fails to delete a subpool
I can't see why those functions should be using sub-pools: I can't see any
repetition or very large memory allocations. Can we get rid of their sub-pools?
> I don't understand the significance of this one:
>
> - libsvn_wc/status.c:close_edit
> fails to clear traversal_info
I haven't looked because I'm pretty sure I won't understand either.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: SVN_ERR bypassing "goto cleanup" [was: svn commit: r14643 - ...]
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
> Erik Huelsmann wrote:
>> On 5/11/05, Julian Foad <ju...@btopenworld.com> wrote:
>>>> - libsvn_wc/questions.c:svn_wc_text_modified_p
>>>> - libsvn_wc/props.c:svn_wc_props_modified_p
>>>> fails to delete a subpool
>>>
>>> I can't see why those functions should be using sub-pools: I can't see any
>>> repetition or very large memory allocations. Can we get rid of their sub-pools?
>>
>> Well, if we're allocating really large chuncks of memory, we sometimes
>> use subpools [...]
>
> Do you think these functions _do_ allocate large amounts of memory?
Here's the log message from when the subpool was introduced:
> ------------------------------------------------------------------------
> r205 | cmpilato | 2001-10-04 01:10:11 +0100 (Thu, 04 Oct 2001) | 23 lines
>
> Sorry, but 10 megs of memory just to run `svn st' on the subversion
> source tree is ridiculous. This brought my test case down below 400k.
>
> * subversion/libsvn_wc/questions.c
>
> (svn_wc_text_modified_p, svn_wc_props_modified_p,
> svn_wc_conflicted_p, svn_wc_has_binary_prop): Use a subpool, and
> destroy it when we're done. For crying out loud, we're returning
> data allocated outside the function. Also, use the SVN_ERR macro.
>
> (contents_identical_p): 80-column formatting.
>
> (timestamps_equal_p): Use SVN_ERR macro.
>
> * subversion/libsvn_wc/status.c
>
> (assemble_status): Copy the entry into the hash's pool.
>
> (svn_wc_statuses): Use a subpool for all non-hash-related
> allocations, destroying it when we're done. Also, use the SVN_ERR
> macro.
> ------------------------------------------------------------------------
I don't understand the comment about "returning data allocated outside the
function", and I still can't see any large allocations in the pool either then
or now in those two functions (svn_wc_text_modified_p,
svn_wc_props_modified_p), except that svn_wc_props_modified_p loads the base
and working sets of properties, which may be largeish. Do we regard two sets
of properties as large enough to warrant a sub-pool? I wouldn't have thought
so, really. Even if in the exceptional case the properties occupy megabytes,
this is only a problem when there is iteration in which case there will be an
iteration sub-pool.
C-Mike or anyone, can you explain or comment? It seems to me that each of
these functions should lose its subpool.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: SVN_ERR bypassing "goto cleanup" [was: svn commit: r14643 - ...]
Posted by Julian Foad <ju...@btopenworld.com>.
Erik Huelsmann wrote:
> On 5/11/05, Julian Foad <ju...@btopenworld.com> wrote:
>>>- libsvn_wc/questions.c:svn_wc_text_modified_p
>>>- libsvn_wc/props.c:svn_wc_props_modified_p
>>> fails to delete a subpool
>>
>>I can't see why those functions should be using sub-pools: I can't see any
>>repetition or very large memory allocations. Can we get rid of their sub-pools?
>
> Well, if we're allocating really large chuncks of memory, we sometimes
> use subpools [...]
Do you think these functions _do_ allocate large amounts of memory?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: SVN_ERR bypassing "goto cleanup" [was: svn commit: r14643 - ...]
Posted by Erik Huelsmann <eh...@gmail.com>.
On 5/11/05, Julian Foad <ju...@btopenworld.com> wrote:
> [Making this thread more visible and following up.]
>
> Philip Martin wrote:
> > I decided to have a look at our existing uses of "goto cleanup", just
> > to see how many had SVN_ERRs that bypassed the cleanup.
> >
> > These look like real errors:
> >
> > - libsvn_wc/adm_crawler.c:svn_wc_crawl_revisions2
> > fails to call reporter->abort_report
> >
> > - libsvn_ra_dav/util.c:parsed_request
> > fails to destroy neon structures
> >
> > - libsvn_fs_base/fs.c:base_create
> > - libsvn_fs_base/fs.c:base_open
> > fails to call cleanup_fs
> >
> > - libsvn_client/commit.c:svn_client_commit
> > fails to call editor->abort_edit
>
> Philip fixed those in r14667.
>
> > Leaving a subpool isn't really a problem, so these are not bugs:
> >
> > - libsvn_wc/questions.c:svn_wc_text_modified_p
> > - libsvn_wc/props.c:svn_wc_props_modified_p
> > fails to delete a subpool
>
> I can't see why those functions should be using sub-pools: I can't see any
> repetition or very large memory allocations. Can we get rid of their sub-pools?
Well, if we're allocating really large chuncks of memory, we sometimes
use subpools to deallocate it before returning from the routine. I
think it's nice form to make sure large unused memory chunks are
released from memory if they serve as routine-internal storage. (Large
as in 128k or so...)
> > I don't understand the significance of this one:
> >
> > - libsvn_wc/status.c:close_edit
> > fails to clear traversal_info
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: SVN_ERR bypassing "goto cleanup" [was: svn commit: r14643 - ...]
Posted by kf...@collab.net.
kfogel@collab.net writes:
> > > I don't understand the significance of this one:
> > > - libsvn_wc/status.c:close_edit
> > > fails to clear traversal_info
> >
> > I haven't looked because I'm pretty sure I won't understand either.
>
> I know the traversal_info stuff somewhat, I'll take a look at this.
Yup, it looks like this was a problem, though a minor one.
See issue #1741 and r8842+r8962 for why the traversal_info needs to be
cleared at all. Unfortunately, clearing it at the beginning of the
function doesn't work, and although it's likely that by returning an
error we'd be making traversal_info irrelevant anyway (because instead
of status being reported, an error would be reported), that's up to
the callers and isn't an assumption close_edit() can make.
It seems like the Most Right Behavior is for close_edit() to ensure
that traversal_info is correct by return time, whether or not we're
returning an error. I've done that in r14709, even though it means
introducing one of those brittle manual-error-checking situations.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: SVN_ERR bypassing "goto cleanup" [was: svn commit: r14643 - ...]
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> > I don't understand the significance of this one:
> > - libsvn_wc/status.c:close_edit
> > fails to clear traversal_info
>
> I haven't looked because I'm pretty sure I won't understand either.
I know the traversal_info stuff somewhat, I'll take a look at this.
(And thanks for these followups, Julian!)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org