Re: svn commit: rev 5147 - trunk/subversion/libsvn_fs

On Thu, Feb 27, 2003 at 11:13:33PM -0600, cmpilato@tigris.org wrote:
>...
> +++ trunk/subversion/libsvn_fs/tree.c Thu Feb 27 23:13:09 2003
>...
> +/* Deltify ID's predecessor iff ID is mutable under TXN_ID in FS. If
> + ID is a mutable directory, recurse. Do this as part of TRAIL. */
> +static svn_error_t *
> +deltify_mutable (svn_fs_t *fs,

That comment doesn't match the function.

>...
> + svn_fs_root_t *root,
> + const char *path,

I think this can be changed to: dag_node_t *node.

> + const char *txn_id,
> + apr_pool_t *pool)
> +{
> + const svn_fs_id_t *id;
> + apr_hash_t *entries = NULL;
> int is_dir;
> + struct txn_deltify_args td_args;
> +
> + /* Get the ID for PATH under ROOT. */
> + SVN_ERR (svn_fs_node_id (&id, root, path, pool));

So this becomes svn_fs__dag_get_id(), but we may not need the id (see below).

> - /* Not mutable? Go no further. This is safe to do because for
> - items in the tree to be mutable, their parent dirs must also be
> - mutable. Therefore, if a directory is not mutable under TXN_ID,
> - its children cannot be. */
> + /* Check for mutability. Not mutable? Go no further. This is safe
> + to do because for items in the tree to be mutable, their parent
> + dirs must also be mutable. Therefore, if a directory is not
> + mutable under TXN_ID, its children cannot be. */
> if (strcmp (svn_fs__id_txn_id (id), txn_id))
> return SVN_NO_ERROR;

This would be svn_fs__dag_check_mutable(node). [because we have this helper,
we don't really need the ID]

> - /* Get the NODE and node revision for ID. */
> - SVN_ERR (svn_fs__dag_get_node (&node, fs, id, trail));
> - SVN_ERR (svn_fs__bdb_get_node_revision (&noderev, fs, id, trail));
> + /* Is this a directory? */
> + SVN_ERR (svn_fs_is_dir (&is_dir, root, path, pool));

This becomes svn_fs__dag_is_directory(node).

>
> - /* If this is a directory, recurse on its entries. */
> - if ((is_dir = svn_fs__dag_is_directory (node)))
> - {
> - SVN_ERR (svn_fs__dag_dir_entries (&entries, node, trail));
> - if (entries)
> - {
> - apr_hash_index_t *hi;
> + /* If this is a directory, read its entries. */
> + if (is_dir)
> + SVN_ERR (svn_fs_dir_entries (&entries, root, path, pool));

This would need a little txn_body wrapper to call svn_fs__dag_dir_entries()
with the node and a trail argument.

>...
> + /* If there are entries, recurse on 'em. */
> + if (entries)
> + {

This conditional is not necessary, as svn_fs_dir_entries() will always
return non-NULL entries. However, if we switch to dag_dir_entries, then this
check is important.

> + apr_pool_t *subpool = svn_pool_create (pool);
> + apr_hash_index_t *hi;
>
> - SVN_ERR (deltify_if_mutable_under_txn_id (fs, dirent->id,
> - txn_id, trail));
> - }
> + for (hi = apr_hash_first (pool, entries); hi; hi = apr_hash_next (hi))
> + {
> + /* KEY will be the entry name, VAL the dirent (about
> + which we really don't care) */
> + const void *key;
> + apr_hash_this (hi, &key, NULL, NULL);

Actually, keep the VAL. It has the child node's id. You can then use
svn_fs__dag_get_node() [within a trail] to get the node for the recursion.

> + SVN_ERR (deltify_mutable (fs, root,
> + svn_path_join (path, key, subpool),
> + txn_id, subpool));

And pass that node here.

> }
> }
>
> - if (noderev->predecessor_id)
> - SVN_ERR (txn_deltify (node, noderev->predecessor_count, is_dir, trail));
> + /* Finally, do the deltification. */
> + td_args.fs = fs;
> + td_args.root = root;
> + td_args.path = path;
> + td_args.is_dir = is_dir;
> + SVN_ERR (svn_fs__retry_txn (fs, txn_body_txn_deltify, &td_args, pool));

You can then pass the node into txn_body_txn_deltify, and it won't have to
go through the whole get_dag() call.

With the above changes, you prevent a *lot* of path traversal by eliminating
the calls to svn_fs_FOO(). Using the dag nodes directly helps a lot.

>...
> + /* Get the new revision root. */
> + SVN_ERR_W (svn_fs_revision_root (&rev_root, fs, *new_rev, pool),
> + "Commit succeeded, but error getting new revision root");
> +
> + /* Now...deltify! */
> + SVN_ERR_W (deltify_mutable (fs, rev_root, "/", txn_id, pool),
> + "Commit succeeded; deltification failed");

Use txn_body_get_root to get the dag node for passing to deltify_mutable.

One note: should you *not* want to go in this direction :-(, then I'd at
least recommend changing the "/" above to just "". txn_body_get_root uses
the empty string to represent root. Consistency between the two would be
Goodness.

In short, if there were a couple little txn_body functions to fetch dir
entries and to fetch nodes-by-id, then this function could be tricked up
quite nicely. I bet those new txn_body functions could be helpful elsewhere
to optimize their operation [but that's a guess; I haven't looked].

Overall, it looks like we'd save one trail-operation by avoiding a call to
svn_fs_is_dir(). An initial view would also say one operation for the
svn_fs_node_id() call, but we lose that in fetching the dag_node in the
recursion loop.

What do you think?

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org