> -----Original Message-----
> From: stefan2_at_apache.org [mailto:stefan2_at_apache.org]
> Sent: zondag 13 april 2014 11:45
> To: commits_at_subversion.apache.org
> Subject: svn commit: r1586947 - in
> /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
>
> Author: stefan2
> Date: Sun Apr 13 09:45:03 2014
> New Revision: 1586947
>
> URL: http://svn.apache.org/r1586947
> Log:
> Improve MT scalability of the FSFS DAG traversal code.
>
> Error objects are a very expensive way to control the control flow
> as they carry their own pools, created from a thread-safe root pool.
>
> dag_open should not return an error to open_path if the dirent
> cannot be found, pass a NULL node back instead. This eliminates
> about 50% of all transitional error objects during log-y operations.
> As there are only 2 callers of dag_open, they are easy to adapt.
Unless there are a lot of string creations to create a vey specific error message, I've never seen a construction of svn_error_t * in any performance traces...
Unless you have some numbers to prove that this helps on more setups than yours, I don't see this as a *single reason* to change current behavior.
There could be valid other reasons of course.
This looks like another case of micro-optimizing certain code which performance critical... I think there is more to gain with other kinds of optimizations, like changing total algorithms to scale better. It wouldn't be the first case where you optimized code that shouldn't have been executed in the first place.
Note that there is still an open thread on dev_at_s.a.o about reverting a lot of changes like this in fsfs for guaranteed stability. I would say this falls in this same discussion if we decide to revert those.
Looking further in your patch makes it appear that this is just a minor refactoring... Not sure if the summary really makes sense for that as I don't think it will provide a measurable performance improvement, while the summary makes it appear that it resolves major multithreading problems.
Bert
>
> * subversion/libsvn_fs_fs/dag.h
> (svn_fs_fs__dag_clone_root): Document the new error behavior.
>
> * subversion/libsvn_fs_fs/dag.c
> (svn_fs_fs__dag_clone_child): This caller actually wants an error,
> thus we create it here.
> (svn_fs_fs__dag_open): Return NULL if entry cannot be found.
>
> * subversion/libsvn_fs_fs/tree.c
> (open_path): Replace error meddling with a check for NULL.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/dag.c
> subversion/trunk/subversion/libsvn_fs_fs/dag.h
> subversion/trunk/subversion/libsvn_fs_fs/tree.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/dag.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/da
> g.c?rev=1586947&r1=1586946&r2=1586947&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/dag.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/dag.c Sun Apr 13 09:45:03
> 2014
> @@ -685,6 +685,10 @@ svn_fs_fs__dag_clone_child(dag_node_t **
>
> /* Find the node named NAME in PARENT's entries list if it exists. */
> SVN_ERR(svn_fs_fs__dag_open(&cur_entry, parent, name, pool,
> subpool));
> + if (! cur_entry)
> + return svn_error_createf
> + (SVN_ERR_FS_NOT_FOUND, NULL,
> + "Attempted to open non-existent child node '%s'", name);
>
> /* Check for mutability in the node we found. If it's mutable, we
> don't need to clone it. */
> @@ -1174,9 +1178,10 @@ svn_fs_fs__dag_open(dag_node_t **child_p
> SVN_ERR(dir_entry_id_from_node(&node_id, parent, name,
> scratch_pool, scratch_pool));
> if (! node_id)
> - return svn_error_createf
> - (SVN_ERR_FS_NOT_FOUND, NULL,
> - "Attempted to open non-existent child node '%s'", name);
> + {
> + *child_p = NULL;
> + return SVN_NO_ERROR;
> + }
>
> /* Make sure that NAME is a single path component. */
> if (! svn_path_is_single_path_component(name))
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/dag.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/da
> g.h?rev=1586947&r1=1586946&r2=1586947&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/dag.h (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/dag.h Sun Apr 13 09:45:03
> 2014
> @@ -254,7 +254,8 @@ svn_error_t *svn_fs_fs__dag_clone_root(d
>
> /* Open the node named NAME in the directory PARENT. Set *CHILD_P to
> the new node, allocated in RESULT_POOL. NAME must be a single path
> - component; it cannot be a slash-separated directory path.
> + component; it cannot be a slash-separated directory path. If NAME does
> + not exist within PARENT, set *CHILD_P to NULL.
> */
> svn_error_t *
> svn_fs_fs__dag_open(dag_node_t **child_p,
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tr
> ee.c?rev=1586947&r1=1586946&r2=1586947&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Sun Apr 13 09:45:03
> 2014
> @@ -1033,7 +1033,6 @@ open_path(parent_path_t **parent_path_p,
> {
> copy_id_inherit_t inherit;
> const char *copy_path = NULL;
> - svn_error_t *err = SVN_NO_ERROR;
> dag_node_t *cached_node = NULL;
>
> /* If we found a directory entry, follow it. First, we
> @@ -1047,17 +1046,15 @@ open_path(parent_path_t **parent_path_p,
> if (cached_node)
> child = cached_node;
> else
> - err = svn_fs_fs__dag_open(&child, here, entry, pool, iterpool);
> + SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool));
>
> /* "file not found" requires special handling. */
> - if (err && err->apr_err == SVN_ERR_FS_NOT_FOUND)
> + if (child == NULL)
> {
> /* If this was the last path component, and the caller
> said it was optional, then don't return an error;
> just put a NULL node pointer in the path. */
>
> - svn_error_clear(err);
> -
> if ((flags & open_path_last_optional)
> && (! next || *next == '\0'))
> {
> @@ -1073,9 +1070,6 @@ open_path(parent_path_t **parent_path_p,
> }
> }
>
> - /* Other errors we return normally. */
> - SVN_ERR(err);
> -
> if (flags & open_path_node_only)
> {
> /* Shortcut: the caller only wan'ts the final DAG node. */
>
Received on 2014-04-13 18:18:49 CEST