On Wed, Mar 4, 2015 at 7:06 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> On 2 January 2014 at 17:01, <stefan2_at_apache.org> wrote:
> > Author: stefan2
> > Date: Thu Jan 2 14:01:43 2014
> > New Revision: 1554807
> >
> > URL: http://svn.apache.org/r1554807
> > Log:
> > Replace the use of svn_fs_node_id + svn_fs_compare_ids with
> > simple calls to svn_fs_node_relation.
> >
> Stefan,
>
> I was looking to one of compiler warnings and noticed unbounded memory
> usage problem in this commit. See my review inline.
>
> [...]
>
> > Modified: subversion/trunk/subversion/libsvn_repos/rev_hunt.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/rev_hunt.c?rev=1554807&r1=1554806&r2=1554807&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_repos/rev_hunt.c (original)
> > +++ subversion/trunk/subversion/libsvn_repos/rev_hunt.c Thu Jan 2
> 14:01:43 2014
> > @@ -309,11 +309,11 @@ svn_repos_deleted_rev(svn_fs_t *fs,
> > apr_pool_t *pool)
> > {
> > apr_pool_t *subpool;
> > - svn_fs_root_t *root, *copy_root;
> > + svn_fs_root_t *start_root, *root, *copy_root;
> > const char *copy_path;
> > svn_revnum_t mid_rev;
> > - const svn_fs_id_t *start_node_id, *curr_node_id;
> > - svn_error_t *err;
> > + svn_node_kind_t kind;
> > + svn_fs_node_relation_t node_relation;
> >
> > /* Validate the revision range. */
> > if (! SVN_IS_VALID_REVNUM(start))
> > @@ -334,32 +334,19 @@ svn_repos_deleted_rev(svn_fs_t *fs,
> > }
> >
> > /* Ensure path exists in fs at start revision. */
> > - SVN_ERR(svn_fs_revision_root(&root, fs, start, pool));
> > - err = svn_fs_node_id(&start_node_id, root, path, pool);
> > - if (err)
> > + SVN_ERR(svn_fs_revision_root(&start_root, fs, start, pool));
> > + SVN_ERR(svn_fs_check_path(&kind, start_root, path, pool));
> > + if (kind == svn_node_none)
> > {
> > - if (err->apr_err == SVN_ERR_FS_NOT_FOUND)
> > - {
> > - /* Path must exist in fs at start rev. */
> > - *deleted = SVN_INVALID_REVNUM;
> > - svn_error_clear(err);
> > - return SVN_NO_ERROR;
> > - }
> > - return svn_error_trace(err);
> > + /* Path must exist in fs at start rev. */
> > + *deleted = SVN_INVALID_REVNUM;
> > + return SVN_NO_ERROR;
> > }
> >
> > /* Ensure path was deleted at or before end revision. */
> > SVN_ERR(svn_fs_revision_root(&root, fs, end, pool));
> > - err = svn_fs_node_id(&curr_node_id, root, path, pool);
> > - if (err && err->apr_err == SVN_ERR_FS_NOT_FOUND)
> > - {
> > - svn_error_clear(err);
> > - }
> > - else if (err)
> > - {
> > - return svn_error_trace(err);
> > - }
> > - else
> > + SVN_ERR(svn_fs_check_path(&kind, root, path, pool));
> > + if (kind != svn_node_none)
> > {
> > /* path exists in the end node and the end node is equivalent
> > or otherwise equivalent to the start node. This can mean
> > @@ -386,8 +373,9 @@ svn_repos_deleted_rev(svn_fs_t *fs,
> > 5) The start node was deleted and replaced by a node which
> > it does not share any history with.
> > */
> > - SVN_ERR(svn_fs_node_id(&curr_node_id, root, path, pool));
> > - if (svn_fs_compare_ids(start_node_id, curr_node_id) != -1)
> > + SVN_ERR(svn_fs_node_relation(&node_relation, start_root, path,
> > + root, path, pool));
> > + if (node_relation != svn_fs_node_unrelated)
> > {
> > SVN_ERR(svn_fs_closest_copy(©_root, ©_path, root,
> > path, pool));
> > @@ -450,28 +438,23 @@ svn_repos_deleted_rev(svn_fs_t *fs,
> >
> > /* Get revision root and node id for mid_rev at that revision. */
> > SVN_ERR(svn_fs_revision_root(&root, fs, mid_rev, subpool));
> > - err = svn_fs_node_id(&curr_node_id, root, path, subpool);
> > -
> > - if (err)
> > + SVN_ERR(svn_fs_check_path(&kind, root, path, pool));
> You're using wrong pool here: POOL instead of SUBPOOL (which is
> actually iterpool). As result this function consume unbounded amount
> of memory.
>
Apart from using the wrong pool and being a bug in that sense,
it should only be run O(log rev) times. Did you actually observe
unbound memory usage?
> > + if (kind == svn_node_none)
> > {
> > - if (err->apr_err == SVN_ERR_FS_NOT_FOUND)
> > - {
> > - /* Case D: Look lower in the range. */
> > - svn_error_clear(err);
> > - end = mid_rev;
> > - mid_rev = (start + mid_rev) / 2;
> > - }
> > - else
> > - return svn_error_trace(err);
> > + /* Case D: Look lower in the range. */
> > + end = mid_rev;
> > + mid_rev = (start + mid_rev) / 2;
> > }
> > else
> > {
> > /* Determine the relationship between the start node
> > and the current node. */
> > - int cmp = svn_fs_compare_ids(start_node_id, curr_node_id);
> > + SVN_ERR(svn_fs_node_relation(&node_relation, start_root, path,
> > + root, path, pool));
> Same here.
>
> > + if (node_relation != svn_fs_node_unrelated)
> > SVN_ERR(svn_fs_closest_copy(©_root, ©_path, root,
> > path, subpool));
> > - if (cmp == -1 ||
> > + if (node_relation == svn_fs_node_unrelated ||
> > (copy_root &&
> > (svn_fs_revision_root_revision(copy_root) > start)))
> > {
>
> I've fixed these problems in r1664084 and nominated to 1.9.x branch.
>
Yes, I've seen that commit. Thank you for fixing that.
-- Stefan^2.
Received on 2015-03-04 22:00:42 CET