On Wed, Mar 4, 2015 at 7:06 PM, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 2 January 2014 at 17:01, <stef...@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.
