Hi Bert,
you gave the hint about per-dir optimization, so if you can spare the time,
could you look at r1158491 and see whether you have some more hints?
Thanks!
~Neels
On 08/17/2011 04:45 AM, neels_at_apache.org wrote:
> Author: neels
> Date: Wed Aug 17 02:45:42 2011
> New Revision: 1158491
>
> URL: http://svn.apache.org/viewvc?rev=1158491&view=rev
> Log:
> Follow-up to r1157292.
> Optimize obtaining moved-to and moved-from for status by querying per-dir
> where possible (suggested by: rhujiben). The original patch is modified so
> that the status structs for nodes return moved-* information *only* on the
> roots of a move, and *not* on their moved-along children. (I had intended to
> return these on all moved nodes, but that does not perform well at all.)
>
> * subversion/include/svn_client.h (svn_client_status_t),
> * subversion/include/svn_wc.h (svn_wc_status3_t):
> Remove MOVED_TO_OP_ROOT_ABSPATH. Tweak comments for the other two MOVED_*
> members (identical changes to both of these status related structs).
>
> * subversion/libsvn_client/status.c
> (svn_client_status_dup, svn_client__create_status):
> Apply removal of MOVED_TO_OP_ROOT_ABSPATH.
>
> * subversion/libsvn_wc/status.c
> (read_info):
> Use suboptimal ways to get MOVED_TO_ABSPATH and MOVED_HERE flag, until
> better ways have been charted (only done for non-recursive status).
> (assemble_status):
> Remove the expensive scan_* calls for the benefit of the information
> available from the per-dir status query. Scan the full MOVED_FROM_ABSPATH
> *only* when MOVED_HERE is TRUE on an op-root.
> (svn_wc_dup_status3):
> Apply removal of MOVED_TO_OP_ROOT_ABSPATH.
>
> * subversion/libsvn_wc/wc_db.c
> (read_children_info):
> Return MOVED_TO_ABSPATH and MOVED_HERE via the modified
> STMT_SELECT_NODE_CHILDREN_INFO query (s.b.).
>
> * subversion/libsvn_wc/wc_db.h
> (svn_wc__db_info_t): Add MOVED_TO_ABSPATH and MOVED_HERE.
>
> * subversion/libsvn_wc/wc-queries.sql
> (STMT_SELECT_NODE_CHILDREN_INFO):
> Also select columns MOVED_HERE and MOVED_TO.
>
> * subversion/svn/status.c
> (print_status):
> Simplify condition as that decision is now taken elsewhere.
>
>
> Modified:
> subversion/trunk/subversion/include/svn_client.h
> subversion/trunk/subversion/include/svn_wc.h
> subversion/trunk/subversion/libsvn_client/status.c
> subversion/trunk/subversion/libsvn_wc/status.c
> subversion/trunk/subversion/libsvn_wc/wc-queries.sql
> subversion/trunk/subversion/libsvn_wc/wc_db.c
> subversion/trunk/subversion/libsvn_wc/wc_db.h
> subversion/trunk/subversion/svn/status.c
>
> Modified: subversion/trunk/subversion/include/svn_client.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=1158491&r1=1158490&r2=1158491&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_client.h (original)
> +++ subversion/trunk/subversion/include/svn_client.h Wed Aug 17 02:45:42 2011
> @@ -2207,18 +2207,38 @@ typedef struct svn_client_status_t
> const void *backwards_compatibility_baton;
>
> /** Set to the local absolute path that this node was moved from, if this
> - * file or directory has been moved here locally. */
> + * file or directory has been moved here locally and is the root of that
> + * move. Otherwise set to NULL.
> + *
> + * This will be NULL for moved-here nodes that are just part of a subtree
> + * that was moved along (and are not themselves a root of a different move
> + * operation).
> + *
> + * @since New in 1.8. */
> const char *moved_from_abspath;
>
> /** Set to the local absolute path that this node was moved to, if this file
> - * or directory has been moved away locally. */
> + * or directory has been moved away locally and corresponds to the root
> + * of the destination side of the move. Otherwise set to NULL.
> + *
> + * Note: Saying just "root" here could be misleading. For example:
> + * svn mv A AA;
> + * svn mv AA/B BB;
> + * creates a situation where A/B is moved-to BB, but one could argue that
> + * the move source's root actually was AA/B. Note that, as far as the
> + * working copy is concerned, above case is exactly identical to:
> + * svn mv A/B BB;
> + * svn mv A AA;
> + * In both situations, @a moved_to_abspath would be set for nodes A (moved
> + * to AA) and A/B (moved to BB), only.
> + *
> + * This will be NULL for moved-away nodes that were just part of a subtree
> + * that was moved along (and are not themselves a root of a different move
> + * operation).
> + *
> + * @since New in 1.8. */
> const char *moved_to_abspath;
>
> - /* If this file or directory has been moved away locally, set this to the
> - * local absolute path that was the root of the move-away, i.e. to the
> - * op-root of the delete-half of the move operation. */
> - const char *moved_to_op_root_abspath;
> -
> /* NOTE! Please update svn_client_status_dup() when adding new fields here. */
> } svn_client_status_t;
>
>
> Modified: subversion/trunk/subversion/include/svn_wc.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_wc.h?rev=1158491&r1=1158490&r2=1158491&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_wc.h (original)
> +++ subversion/trunk/subversion/include/svn_wc.h Wed Aug 17 02:45:42 2011
> @@ -3618,18 +3618,38 @@ typedef struct svn_wc_status3_t
> /** @} */
>
> /** Set to the local absolute path that this node was moved from, if this
> - * file or directory has been moved here locally. */
> + * file or directory has been moved here locally and is the root of that
> + * move. Otherwise set to NULL.
> + *
> + * This will be NULL for moved-here nodes that are just part of a subtree
> + * that was moved along (and are not themselves a root of a different move
> + * operation).
> + *
> + * @since New in 1.8. */
> const char *moved_from_abspath;
>
> /** Set to the local absolute path that this node was moved to, if this file
> - * or directory has been moved away locally. */
> + * or directory has been moved away locally and corresponds to the root
> + * of the destination side of the move. Otherwise set to NULL.
> + *
> + * Note: Saying just "root" here could be misleading. For example:
> + * svn mv A AA;
> + * svn mv AA/B BB;
> + * creates a situation where A/B is moved-to BB, but one could argue that
> + * the move source's root actually was AA/B. Note that, as far as the
> + * working copy is concerned, above case is exactly identical to:
> + * svn mv A/B BB;
> + * svn mv A AA;
> + * In both situations, @a moved_to_abspath would be set for nodes A (moved
> + * to AA) and A/B (moved to BB), only.
> + *
> + * This will be NULL for moved-away nodes that were just part of a subtree
> + * that was moved along (and are not themselves a root of a different move
> + * operation).
> + *
> + * @since New in 1.8. */
> const char *moved_to_abspath;
>
> - /* If this file or directory has been moved away locally, set this to the
> - * local absolute path that was the root of the move-away, i.e. to the
> - * op-root of the delete-half of the move operation. */
> - const char *moved_to_op_root_abspath;
> -
> /* NOTE! Please update svn_wc_dup_status3() when adding new fields here. */
> } svn_wc_status3_t;
>
>
> Modified: subversion/trunk/subversion/libsvn_client/status.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/status.c?rev=1158491&r1=1158490&r2=1158491&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/status.c (original)
> +++ subversion/trunk/subversion/libsvn_client/status.c Wed Aug 17 02:45:42 2011
> @@ -570,10 +570,6 @@ svn_client_status_dup(const svn_client_s
> if (status->moved_to_abspath)
> st->moved_to_abspath = apr_pstrdup(result_pool, status->moved_to_abspath);
>
> - if (status->moved_to_op_root_abspath)
> - st->moved_to_op_root_abspath =
> - apr_pstrdup(result_pool, status->moved_to_op_root_abspath);
> -
> return st;
> }
>
> @@ -680,7 +676,6 @@ svn_client__create_status(svn_client_sta
>
> (*cst)->moved_from_abspath = status->moved_from_abspath;
> (*cst)->moved_to_abspath = status->moved_to_abspath;
> - (*cst)->moved_to_op_root_abspath = status->moved_to_op_root_abspath;
>
> return SVN_NO_ERROR;
> }
>
> Modified: subversion/trunk/subversion/libsvn_wc/status.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/status.c?rev=1158491&r1=1158490&r2=1158491&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/status.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/status.c Wed Aug 17 02:45:42 2011
> @@ -273,6 +273,68 @@ read_info(const struct svn_wc__db_info_t
> &mtb->lock, NULL, NULL,
> db, local_abspath,
> result_pool, scratch_pool));
> +
> + if (mtb->status == svn_wc__db_status_deleted)
> + {
> + const char *moved_to_abspath;
> + const char *moved_to_op_root_abspath;
> +
> + /* NOTE: we can't use op-root-ness as a condition here since a base
> + * node can be the root of a move and still not be an explicit
> + * op-root (having a working node with op_depth == pathelements).
> + *
> + * Both these (almost identical) situations showcase this:
> + * svn mv a/b bb
> + * svn del a
> + * and
> + * svn mv a aa
> + * svn mv aa/b bb
> + * In both, 'bb' is moved from 'a/b', but 'a/b' has no op_depth>0
> + * node at all, as its parent 'a' is locally deleted. */
> +
> + SVN_ERR(svn_wc__db_scan_deletion(NULL,
> + &moved_to_abspath,
> + NULL,
> + &moved_to_op_root_abspath,
> + db, local_abspath,
> + scratch_pool, scratch_pool));
> + if (moved_to_abspath != NULL
> + && moved_to_op_root_abspath != NULL
> + && strcmp(moved_to_abspath, moved_to_op_root_abspath) == 0)
> + {
> + mtb->moved_to_abspath = apr_pstrdup(result_pool,
> + moved_to_abspath);
> + }
> + /* ### ^^^ THIS SUCKS. For at least two reasons:
> + * 1) We scan the node deletion and that's technically not necessary.
> + * We'd be fine to know if this is an actual root of a move.
> + * 2) From the elaborately calculated results, we backwards-guess
> + * whether this is a root.
> + * It works ok, and this code only gets called when a node is an
> + * explicit target of a 'status'. But it would be better to do this
> + * differently.
> + * We could return moved-to via svn_wc__db_base_get_info() (called
> + * just above), but as moved-to is only intended to be returned for
> + * roots of a move, that doesn't fit too well. */
> + }
> + }
> +
> + /* ### svn_wc__db_read_info() could easily return the moved-here flag. But
> + * for now... (The per-dir query for recursive status is far more optimal.)
> + * Note that this actually scans around to get the full path, for a bool.
> + * This bool then gets returned, later is evaluated, and if true leads to
> + * the same paths being scanned again. We'd want to obtain this bool here as
> + * cheaply as svn_wc__db_read_children_info() does. */
> + if (mtb->status == svn_wc__db_status_added)
> + {
> + const char *moved_from_abspath = NULL;
> + SVN_ERR(svn_wc__db_scan_addition(NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL,
> + &moved_from_abspath,
> + NULL,
> + db, local_abspath,
> + result_pool, scratch_pool));
> + mtb->moved_here = (moved_from_abspath != NULL);
> }
>
> mtb->has_checksum = (checksum != NULL);
> @@ -404,8 +466,6 @@ assemble_status(svn_wc_status3_t **statu
> const char *repos_root_url;
> const char *repos_uuid;
> const char *moved_from_abspath = NULL;
> - const char *moved_to_abspath = NULL;
> - const char *moved_to_op_root_abspath = NULL;
> svn_filesize_t filesize = (dirent && (dirent->kind == svn_node_file))
> ? dirent->filesize
> : SVN_INVALID_FILESIZE;
> @@ -608,26 +668,20 @@ assemble_status(svn_wc_status3_t **statu
> else if (schedule == svn_wc_schedule_replace)
> node_status = svn_wc_status_replaced;
> }
> +
> + /* Get moved-from info (only for potential op-roots of a move). */
> + if (node_status == svn_wc_status_added
> + && info->moved_here
> + && info->op_root)
> + SVN_ERR(svn_wc__db_scan_addition(NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL,
> + &moved_from_abspath,
> + NULL,
> + db, local_abspath,
> + result_pool, scratch_pool));
> }
> }
>
> - /* Get moved-to info. */
> - if (info->status == svn_wc__db_status_deleted)
> - SVN_ERR(svn_wc__db_scan_deletion(NULL,
> - &moved_to_abspath,
> - NULL,
> - &moved_to_op_root_abspath,
> - db, local_abspath,
> - result_pool, scratch_pool));
> -
> - /* Get moved-from info. */
> - if (info->status == svn_wc__db_status_added)
> - SVN_ERR(svn_wc__db_scan_addition(NULL, NULL, NULL, NULL, NULL,
> - NULL, NULL, NULL, NULL,
> - &moved_from_abspath,
> - NULL,
> - db, local_abspath,
> - result_pool, scratch_pool));
>
> if (node_status == svn_wc_status_normal)
> node_status = text_status;
> @@ -722,8 +776,7 @@ assemble_status(svn_wc_status3_t **statu
> stat->repos_uuid = repos_uuid;
>
> stat->moved_from_abspath = moved_from_abspath;
> - stat->moved_to_abspath = moved_to_abspath;
> - stat->moved_to_op_root_abspath = moved_to_op_root_abspath;
> + stat->moved_to_abspath = info->moved_to_abspath;
>
> *status = stat;
>
> @@ -2624,10 +2677,6 @@ svn_wc_dup_status3(const svn_wc_status3_
> new_stat->moved_to_abspath
> = apr_pstrdup(pool, orig_stat->moved_to_abspath);
>
> - if (orig_stat->moved_to_op_root_abspath)
> - new_stat->moved_to_op_root_abspath
> - = apr_pstrdup(pool, orig_stat->moved_to_op_root_abspath);
> -
> /* Return the new hotness. */
> return new_stat;
> }
>
> Modified: subversion/trunk/subversion/libsvn_wc/wc-queries.sql
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc-queries.sql?rev=1158491&r1=1158490&r2=1158491&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/wc-queries.sql (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc-queries.sql Wed Aug 17 02:45:42 2011
> @@ -119,7 +119,7 @@ WHERE wc_id = ?1 AND local_relpath = ?2
> SELECT op_depth, nodes.repos_id, nodes.repos_path, presence, kind, revision,
> checksum, translated_size, changed_revision, changed_date, changed_author,
> depth, symlink_target, last_mod_time, properties, lock_token, lock_owner,
> - lock_comment, lock_date, local_relpath
> + lock_comment, lock_date, local_relpath, moved_here, moved_to
> FROM nodes
> LEFT OUTER JOIN lock ON nodes.repos_id = lock.repos_id
> AND nodes.repos_path = lock.repos_relpath
>
> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1158491&r1=1158490&r2=1158491&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Wed Aug 17 02:45:42 2011
> @@ -7038,6 +7038,8 @@ read_children_info(void *baton,
>
> if (op_depth == 0)
> {
> + const char *moved_to_relpath;
> +
> child_item->info.have_base = TRUE;
>
> /* Get the lock info. The query only reports lock info in the row at
> @@ -7045,11 +7047,20 @@ read_children_info(void *baton,
> if (op_depth == 0)
> child_item->info.lock = lock_from_columns(stmt, 15, 16, 17, 18,
> result_pool);
> +
> + /* Moved-to is only stored at op_depth 0. */
> + moved_to_relpath = svn_sqlite__column_text(stmt, 21, NULL);
> + if (moved_to_relpath)
> + child_item->info.moved_to_abspath =
> + svn_dirent_join(wcroot->abspath, moved_to_relpath, result_pool);
> }
> else
> {
> child_item->nr_layers++;
> child_item->info.have_more_work = (child_item->nr_layers > 1);
> +
> + /* Moved-here can only exist at op_depth > 0. */
> + child_item->info.moved_here = svn_sqlite__column_boolean(stmt, 20);
> }
>
> err = svn_sqlite__step(&have_row, stmt);
>
> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.h?rev=1158491&r1=1158490&r2=1158491&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/wc_db.h (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc_db.h Wed Aug 17 02:45:42 2011
> @@ -1842,6 +1842,9 @@ struct svn_wc__db_info_t {
>
> svn_boolean_t locked; /* WC directory lock */
> svn_wc__db_lock_t *lock; /* Repository file lock */
> +
> + const char *moved_to_abspath; /* Only on op-roots. See svn_wc_status3_t. */
> + svn_boolean_t moved_here; /* On both op-roots and children. */
> };
>
> /* Return in *NODES a hash mapping name->struct svn_wc__db_info_t for
>
> Modified: subversion/trunk/subversion/svn/status.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/status.c?rev=1158491&r1=1158490&r2=1158491&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/svn/status.c (original)
> +++ subversion/trunk/subversion/svn/status.c Wed Aug 17 02:45:42 2011
> @@ -272,6 +272,10 @@ print_status(const char *path,
> (*prop_conflicts)++;
> }
>
> + /* Note that moved-from and moved-to information is only available in STATUS
> + * for (op-)roots of a move. Those are exactly the nodes we want to show
> + * move info for in 'svn status'. See also comments in svn_wc_status3_t. */
> +
> if (status->moved_from_abspath)
> {
> const char *cwd;
> @@ -285,13 +289,7 @@ print_status(const char *path,
> (char *)NULL);
> }
>
> - /* Only print an extra moved-to line for the op-root of a move-away.
> - * As each and every child node of a deleted tree is printed in status
> - * output, each of them would be "duplicated" with a moved-to line. */
> - if (status->moved_to_abspath
> - && status->moved_to_op_root_abspath
> - && 0 == strcmp(status->moved_to_op_root_abspath,
> - status->moved_to_abspath))
> + if (status->moved_to_abspath)
> {
> const char *cwd;
> const char *relpath;
>
>
Received on 2011-08-17 22:28:50 CEST