You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2012/11/27 11:53:18 UTC
svn commit: r1414123 - in /subversion/trunk/subversion/libsvn_wc: props.c
status.c wc.h wc_db.c wc_db.h
Author: rhuijben
Date: Tue Nov 27 10:53:16 2012
New Revision: 1414123
URL: http://svn.apache.org/viewvc?rev=1414123&view=rev
Log:
In preparation for further performance improvements, make the retrieval of
inherited properties use a constant number of db operations.
This function is currently used from 'svn status' for every directory that
contains unversioned nodes, which makes it performance critical.
* subversion/libsvn_wc/props.c
(filter_unwanted_props): Move to wc_db.c.
(svn_wc__internal_get_iprops): Move to wc_db.c.
(svn_wc__get_iprops): Wrap svn_wc__internal_get_iprops.
* subversion/libsvn_wc/wc.h
(svn_wc__internal_get_iprops): Remove function.
* subversion/libsvn_wc/wc_db.c
(includes): Add svn_sorts.h.
(filter_unwanted_props): Moved here from props.c
(read_inherited_props_baton_t): New struct.
(db_read_inherited_props): New function, based on
the old svn_wc__internal_get_iprops.
(svn_wc__db_read_inherited_props): New function.
* subversion/libsvn_wc/wc_db.h
(svn_wc__db_read_inherited_props): New function.
Modified:
subversion/trunk/subversion/libsvn_wc/props.c
subversion/trunk/subversion/libsvn_wc/status.c
subversion/trunk/subversion/libsvn_wc/wc.h
subversion/trunk/subversion/libsvn_wc/wc_db.c
subversion/trunk/subversion/libsvn_wc/wc_db.h
Modified: subversion/trunk/subversion/libsvn_wc/props.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/props.c?rev=1414123&r1=1414122&r2=1414123&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/props.c (original)
+++ subversion/trunk/subversion/libsvn_wc/props.c Tue Nov 27 10:53:16 2012
@@ -2338,132 +2338,6 @@ svn_wc__has_magic_property(const apr_arr
return FALSE;
}
-/* Remove all prop name value pairs from PROP_HASH where the property
- name is not PROPNAME. */
-static void
-filter_unwanted_props(apr_hash_t *prop_hash,
- const char * propname,
- apr_pool_t *scratch_pool)
-{
- apr_hash_index_t *hi;
-
- for (hi = apr_hash_first(scratch_pool, prop_hash);
- hi;
- hi = apr_hash_next(hi))
- {
- const char *ipropname = svn__apr_hash_index_key(hi);
-
- if (strcmp(ipropname, propname) != 0)
- apr_hash_set(prop_hash, ipropname, APR_HASH_KEY_STRING, NULL);
- }
- return;
-}
-
-svn_error_t *
-svn_wc__internal_get_iprops(apr_array_header_t **inherited_props,
- svn_wc__db_t *db,
- const char *local_abspath,
- const char *propname,
- apr_pool_t *result_pool,
- apr_pool_t *scratch_pool)
-{
- int i;
- apr_array_header_t *cached_iprops = NULL;
- const char *parent_abspath = local_abspath;
- svn_boolean_t is_wc_root = FALSE;
- apr_pool_t *iterpool = svn_pool_create(scratch_pool);
-
- SVN_ERR_ASSERT(inherited_props);
- *inherited_props = apr_array_make(result_pool, 1,
- sizeof(svn_prop_inherited_item_t *));
-
- /* Walk up to the root of the WC looking for inherited properties. When we
- reach the WC root also check for cached inherited properties. */
- while (TRUE)
- {
- apr_hash_t *actual_props;
- svn_boolean_t is_switched;
-
- svn_pool_clear(iterpool);
-
- SVN_ERR(svn_wc__db_is_switched(&is_wc_root, &is_switched, NULL,
- db, parent_abspath, iterpool));
-
- if (is_switched || is_wc_root)
- {
- is_wc_root = TRUE;
-
- /* If the WC root is also the root of the repository then by
- definition there are no inheritable properties to be had,
- but checking for that is just as expensive as fetching them
- anyway. */
-
- /* Grab the cached inherited properties for the WC root. */
- SVN_ERR(svn_wc__db_read_cached_iprops(&cached_iprops, db,
- parent_abspath,
- scratch_pool, iterpool));
- }
-
- /* If PARENT_ABSPATH is a true parent of LOCAL_ABSPATH, then
- LOCAL_ABSPATH can inherit properties from it. */
- if (strcmp(local_abspath, parent_abspath) != 0)
- {
- SVN_ERR(svn_wc__db_read_props(&actual_props, db, parent_abspath,
- result_pool, iterpool));
- if (actual_props)
- {
- /* If we only want PROPNAME filter out any other properties. */
- if (propname)
- filter_unwanted_props(actual_props, propname, iterpool);
-
- if (apr_hash_count(actual_props))
- {
- svn_prop_inherited_item_t *iprop_elt =
- apr_pcalloc(result_pool,
- sizeof(svn_prop_inherited_item_t));
- iprop_elt->path_or_url = apr_pstrdup(result_pool,
- parent_abspath);
- iprop_elt->prop_hash = actual_props;
- /* Build the output array in depth-first order. */
- svn_sort__array_insert(&iprop_elt, *inherited_props, 0);
- }
- }
- }
-
- /* Inheritance only goes as far as the nearest WC root. */
- if (is_wc_root)
- break;
-
- /* Keep looking for the WC root. */
- parent_abspath = svn_dirent_dirname(parent_abspath, scratch_pool);
- }
-
- if (cached_iprops)
- {
- for (i = cached_iprops->nelts - 1; i >= 0; i--)
- {
- svn_prop_inherited_item_t *cached_iprop =
- APR_ARRAY_IDX(cached_iprops, i, svn_prop_inherited_item_t *);
-
- /* An empty property hash in the iprops cache means there are no
- inherited properties. */
- if (apr_hash_count(cached_iprop->prop_hash) == 0)
- continue;
-
- if (propname)
- filter_unwanted_props(cached_iprop->prop_hash, propname,
- scratch_pool);
-
- /* If we didn't filter everything then keep this iprop. */
- if (apr_hash_count(cached_iprop->prop_hash))
- svn_sort__array_insert(&cached_iprop, *inherited_props, 0);
- }
- }
-
- svn_pool_destroy(iterpool);
- return SVN_NO_ERROR;
-}
-
svn_error_t *
svn_wc__get_iprops(apr_array_header_t **inherited_props,
svn_wc_context_t *wc_ctx,
@@ -2472,9 +2346,11 @@ svn_wc__get_iprops(apr_array_header_t **
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
{
- return svn_error_trace(svn_wc__internal_get_iprops(inherited_props, wc_ctx->db,
- local_abspath, propname,
- result_pool, scratch_pool));
+ return svn_error_trace(
+ svn_wc__db_read_inherited_props(inherited_props,
+ wc_ctx->db, local_abspath,
+ propname,
+ result_pool, scratch_pool));
}
svn_error_t *
Modified: subversion/trunk/subversion/libsvn_wc/status.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/status.c?rev=1414123&r1=1414122&r2=1414123&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/status.c (original)
+++ subversion/trunk/subversion/libsvn_wc/status.c Tue Nov 27 10:53:16 2012
@@ -1028,9 +1028,10 @@ collect_ignore_patterns(apr_array_header
FALSE, result_pool);
}
- SVN_ERR(svn_wc__internal_get_iprops(&inherited_props, db, local_abspath,
- SVN_PROP_INHERITABLE_IGNORES,
- scratch_pool, scratch_pool));
+ SVN_ERR(svn_wc__db_read_inherited_props(&inherited_props,
+ db, local_abspath,
+ SVN_PROP_INHERITABLE_IGNORES,
+ scratch_pool, scratch_pool));
for (i = 0; i < inherited_props->nelts; i++)
{
apr_hash_index_t *hi;
Modified: subversion/trunk/subversion/libsvn_wc/wc.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc.h?rev=1414123&r1=1414122&r2=1414123&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc.h (original)
+++ subversion/trunk/subversion/libsvn_wc/wc.h Tue Nov 27 10:53:16 2012
@@ -607,15 +607,6 @@ svn_wc__internal_get_repos_relpath(const
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
-/* Internal version of svn_wc__get_iprops() */
-svn_error_t *
-svn_wc__internal_get_iprops(apr_array_header_t **inherited_props,
- svn_wc__db_t *db,
- const char *local_abspath,
- const char *propname,
- apr_pool_t *result_pool,
- apr_pool_t *scratch_pool);
-
/* Upgrade the wc sqlite database given in SDB for the wc located at
WCROOT_ABSPATH. It's current/starting format is given by START_FORMAT.
After the upgrade is complete (to as far as the automatic upgrade will
Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1414123&r1=1414122&r2=1414123&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
+++ subversion/trunk/subversion/libsvn_wc/wc_db.c Tue Nov 27 10:53:16 2012
@@ -32,6 +32,7 @@
#include "svn_dirent_uri.h"
#include "svn_path.h"
#include "svn_hash.h"
+#include "svn_sorts.h"
#include "svn_wc.h"
#include "svn_checksum.h"
#include "svn_pools.h"
@@ -9060,6 +9061,175 @@ svn_wc__db_read_cached_iprops(apr_array_
return SVN_NO_ERROR;
}
+/* Remove all prop name value pairs from PROP_HASH where the property
+ name is not PROPNAME. */
+static void
+filter_unwanted_props(apr_hash_t *prop_hash,
+ const char * propname,
+ apr_pool_t *scratch_pool)
+{
+ apr_hash_index_t *hi;
+
+ for (hi = apr_hash_first(scratch_pool, prop_hash);
+ hi;
+ hi = apr_hash_next(hi))
+ {
+ const char *ipropname = svn__apr_hash_index_key(hi);
+
+ if (strcmp(ipropname, propname) != 0)
+ apr_hash_set(prop_hash, ipropname, APR_HASH_KEY_STRING, NULL);
+ }
+ return;
+}
+
+/* Baton for db_read_inherited_props */
+struct read_inherited_props_baton_t
+{
+ apr_array_header_t *iprops;
+ svn_wc__db_t *db;
+ const char *local_abspath;
+ const char *propname;
+ apr_pool_t *result_pool;
+};
+
+/* Implements svn_wc__db_txn_callback_t for svn_wc__db_read_inherited_props */
+static svn_error_t *
+db_read_inherited_props(void *baton,
+ svn_wc__db_wcroot_t *wcroot,
+ const char *local_relpath,
+ apr_pool_t *scratch_pool)
+{
+ struct read_inherited_props_baton_t *ripb = baton;
+ int i;
+ apr_array_header_t *cached_iprops = NULL;
+ const char *parent_abspath = ripb->local_abspath;
+ svn_boolean_t is_wc_root = FALSE;
+ apr_pool_t *iterpool = svn_pool_create(scratch_pool);
+ apr_pool_t *result_pool = ripb->result_pool;
+ svn_wc__db_t *db = ripb->db;
+
+ ripb->iprops = apr_array_make(ripb->result_pool, 1,
+ sizeof(svn_prop_inherited_item_t *));
+
+ /* Walk up to the root of the WC looking for inherited properties. When we
+ reach the WC root also check for cached inherited properties. */
+ while (TRUE)
+ {
+ apr_hash_t *actual_props;
+ svn_boolean_t is_switched;
+
+ svn_pool_clear(iterpool);
+
+ SVN_ERR(svn_wc__db_is_switched(&is_wc_root, &is_switched, NULL,
+ db, parent_abspath, iterpool));
+
+ if (is_switched || is_wc_root)
+ {
+ is_wc_root = TRUE;
+
+ /* If the WC root is also the root of the repository then by
+ definition there are no inheritable properties to be had,
+ but checking for that is just as expensive as fetching them
+ anyway. */
+
+ /* Grab the cached inherited properties for the WC root. */
+ SVN_ERR(svn_wc__db_read_cached_iprops(&cached_iprops, db,
+ parent_abspath,
+ scratch_pool, iterpool));
+ }
+
+ /* If PARENT_ABSPATH is a true parent of LOCAL_ABSPATH, then
+ LOCAL_ABSPATH can inherit properties from it. */
+ if (strcmp(ripb->local_abspath, parent_abspath) != 0)
+ {
+ SVN_ERR(svn_wc__db_read_props(&actual_props, db, parent_abspath,
+ result_pool, iterpool));
+ if (actual_props)
+ {
+ /* If we only want PROPNAME filter out any other properties. */
+ if (ripb->propname)
+ filter_unwanted_props(actual_props, ripb->propname, iterpool);
+
+ if (apr_hash_count(actual_props))
+ {
+ svn_prop_inherited_item_t *iprop_elt =
+ apr_pcalloc(ripb->result_pool,
+ sizeof(svn_prop_inherited_item_t));
+ iprop_elt->path_or_url = apr_pstrdup(result_pool,
+ parent_abspath);
+ iprop_elt->prop_hash = actual_props;
+ /* Build the output array in depth-first order. */
+ svn_sort__array_insert(&iprop_elt, ripb->iprops, 0);
+ }
+ }
+ }
+
+ /* Inheritance only goes as far as the nearest WC root. */
+ if (is_wc_root)
+ break;
+
+ /* Keep looking for the WC root. */
+ parent_abspath = svn_dirent_dirname(parent_abspath, scratch_pool);
+ }
+
+ if (cached_iprops)
+ {
+ for (i = cached_iprops->nelts - 1; i >= 0; i--)
+ {
+ svn_prop_inherited_item_t *cached_iprop =
+ APR_ARRAY_IDX(cached_iprops, i, svn_prop_inherited_item_t *);
+
+ /* An empty property hash in the iprops cache means there are no
+ inherited properties. */
+ if (apr_hash_count(cached_iprop->prop_hash) == 0)
+ continue;
+
+ if (ripb->propname)
+ filter_unwanted_props(cached_iprop->prop_hash, ripb->propname,
+ scratch_pool);
+
+ /* If we didn't filter everything then keep this iprop. */
+ if (apr_hash_count(cached_iprop->prop_hash))
+ svn_sort__array_insert(&cached_iprop, ripb->iprops, 0);
+ }
+ }
+
+ svn_pool_destroy(iterpool);
+ return SVN_NO_ERROR;
+}
+
+svn_error_t *
+svn_wc__db_read_inherited_props(apr_array_header_t **iprops,
+ svn_wc__db_t *db,
+ const char *local_abspath,
+ const char *propname,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ svn_wc__db_wcroot_t *wcroot;
+ const char *local_relpath;
+ struct read_inherited_props_baton_t ripb;
+
+ SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath));
+
+ SVN_ERR(svn_wc__db_wcroot_parse_local_abspath(&wcroot, &local_relpath,
+ db, local_abspath,
+ scratch_pool, scratch_pool));
+ VERIFY_USABLE_WCROOT(wcroot);
+
+ ripb.iprops = NULL;
+ ripb.db = db;
+ ripb.local_abspath = local_abspath;
+ ripb.propname = propname;
+ ripb.result_pool = result_pool;
+
+ SVN_ERR(svn_wc__db_with_txn(wcroot, local_relpath, db_read_inherited_props,
+ &ripb, scratch_pool));
+
+ *iprops = ripb.iprops;
+ return SVN_NO_ERROR;
+}
+
/* Baton for get_children_with_cached_iprops */
struct get_children_with_cached_baton_t
{
Modified: subversion/trunk/subversion/libsvn_wc/wc_db.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.h?rev=1414123&r1=1414122&r2=1414123&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc_db.h (original)
+++ subversion/trunk/subversion/libsvn_wc/wc_db.h Tue Nov 27 10:53:16 2012
@@ -2094,6 +2094,26 @@ svn_wc__db_read_pristine_props(apr_hash_
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
+
+/**
+ * Set @a *inherited_props to a depth-first ordered array of
+ * #svn_prop_inherited_item_t * structures representing the properties
+ * inherited by @a local_abspath from the ACTUAL tree above
+ * @a local_abspath (looking through to the WORKING or BASE tree as
+ * required), up to and including the root of the working copy and
+ * any cached inherited properties inherited by the root.
+ *
+ * Allocate @a *inherited_props in @a result_pool. Use @a scratch_pool
+ * for temporary allocations.
+ */
+svn_error_t *
+svn_wc__db_read_inherited_props(apr_array_header_t **iprops,
+ svn_wc__db_t *db,
+ const char *local_abspath,
+ const char *propname,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
/* Read a BASE node's inherited property information.
Set *IPROPS to to a depth-first ordered array of
Re: r1414123 - optimize iprops retrieval in libsvn_wc
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote:
> On Wed, Nov 28, 2012 at 2:25 PM, Julian Foad wrote:
>>> +/* Remove all prop name value pairs from PROP_HASH where the property
>>> + name is not PROPNAME. */
>>> +static void
>>> +filter_unwanted_props(apr_hash_t *prop_hash,
>>> + const char * propname,
>>> + apr_pool_t *scratch_pool)
>>> +{
>>> + apr_hash_index_t *hi;
>>> +
>>> + for (hi = apr_hash_first(scratch_pool, prop_hash);
>>> + hi;
>>> + hi = apr_hash_next(hi))
>>> + {
>>> + const char *ipropname = svn__apr_hash_index_key(hi);
>>> +
>>> + if (strcmp(ipropname, propname) != 0)
>>> + apr_hash_set(prop_hash, ipropname, APR_HASH_KEY_STRING, NULL);
>>> + }
>>
>> I suppose a much quicker way to select just the wanted key-value pair from
>> the hash is:
>>
>> propval = hash_get(propname)
>> hash_clear()
>
> (Julian - sorry for the tardy reply, I was cleaning out old TODOs
> today and found this)
>
> apr_hash_clear clears the hash by iterating over it just like we do above:
[...]
> I did a quick test: Set 10,000 properties on the root of a WC then
> ran 'svn pg some-prop some-subtree-path --show-inherited-props'.
> Average execution time over 5 runs was .052s for the existing code and
> .056s for your suggested change
IIRC I was thinking how many seconds quicker I could read the code as well as how many nanoseconds quicker I assumed hash_clear would be at run time. That using the latter adds a microsecond [1] surprises me -- and I'm sure apr_hash_clear could be optimized if you care about such things -- but either way you'll have to run "svn pg" an awful lot of times now for it to add up to the hour I've spent looking at this.
> ...so I'll leave the code as is.
OK.
- Julian
[1] Linear extrapolation, assuming << 10 props on a typical directory.
Re: r1414123 - optimize iprops retrieval in libsvn_wc
Posted by Paul Burba <pt...@gmail.com>.
On Wed, Nov 28, 2012 at 2:25 PM, Julian Foad <ju...@btopenworld.com> wrote:
>> URL: http://svn.apache.org/viewvc?rev=1414123&view=rev
>
>> Log:
>> In preparation for further performance improvements, make the
>> retrieval of inherited properties use a constant number of db
>> operations.
>
> My comments are about the existing code, not specifically about this revision.
>
>> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
>> ==============================================================================
>
>> +/* Remove all prop name value pairs from PROP_HASH where the property
>> + name is not PROPNAME. */
>> +static void
>> +filter_unwanted_props(apr_hash_t *prop_hash,
>> + const char * propname,
>> + apr_pool_t *scratch_pool)
>> +{
>> + apr_hash_index_t *hi;
>> +
>> + for (hi = apr_hash_first(scratch_pool, prop_hash);
>> + hi;
>> + hi = apr_hash_next(hi))
>> + {
>> + const char *ipropname = svn__apr_hash_index_key(hi);
>> +
>> + if (strcmp(ipropname, propname) != 0)
>> + apr_hash_set(prop_hash, ipropname, APR_HASH_KEY_STRING, NULL);
>> + }
>
> I suppose a much quicker way to select just the wanted key-value pair from the hash is:
>
> propval = hash_get(propname)
> hash_clear()
(Julian - sorry for the tardy reply, I was cleaning out old TODOs
today and found this)
apr_hash_clear clears the hash by iterating over it just like we do above:
APR_DECLARE(void) apr_hash_clear(apr_hash_t *ht)
{
apr_hash_index_t *hi;
for (hi = apr_hash_first(NULL, ht); hi; hi = apr_hash_next(hi))
apr_hash_set(ht, hi->this->key, hi->this->klen, NULL);
}
So what do we really gain? Your suggestion does do away with the
strcmp for every item in the hash, but we gain a call to apr_hash_set.
I did a quick test: Set 10,000 properties on the root of a WC then
ran 'svn pg some-prop some-subtree-path --show-inherited-props'.
Average execution time over 5 runs was .052s for the existing code and
.056s for your suggested change...so I'll leave the code as is.
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
Re: r1414123 - optimize iprops retrieval in libsvn_wc
Posted by Julian Foad <ju...@btopenworld.com>.
> URL: http://svn.apache.org/viewvc?rev=1414123&view=rev
> Log:
> In preparation for further performance improvements, make the
> retrieval of inherited properties use a constant number of db
> operations.
My comments are about the existing code, not specifically about this revision.
> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
> ==============================================================================
> +/* Remove all prop name value pairs from PROP_HASH where the property
> + name is not PROPNAME. */
> +static void
> +filter_unwanted_props(apr_hash_t *prop_hash,
> + const char * propname,
> + apr_pool_t *scratch_pool)
> +{
> + apr_hash_index_t *hi;
> +
> + for (hi = apr_hash_first(scratch_pool, prop_hash);
> + hi;
> + hi = apr_hash_next(hi))
> + {
> + const char *ipropname = svn__apr_hash_index_key(hi);
> +
> + if (strcmp(ipropname, propname) != 0)
> + apr_hash_set(prop_hash, ipropname, APR_HASH_KEY_STRING, NULL);
> + }
I suppose a much quicker way to select just the wanted key-value pair from the hash is:
propval = hash_get(propname)
hash_clear()
if propval:
hash_set(propname, propval)
> + return;
> +}
[...]
> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.h
> ==============================================================================
> +/**
> + * Set @a *inherited_props to a depth-first ordered array of
"depth-ordered", not depth-first.
> + * #svn_prop_inherited_item_t * structures representing the properties
> + * inherited by @a local_abspath from the ACTUAL tree above
> + * @a local_abspath (looking through to the WORKING or BASE tree as
> + * required), up to and including the root of the working copy and
> + * any cached inherited properties inherited by the root.
> + *
> + * Allocate @a *inherited_props in @a result_pool. Use @a scratch_pool
> + * for temporary allocations.
> + */
> +svn_error_t *
> +svn_wc__db_read_inherited_props(apr_array_header_t **iprops,
> + svn_wc__db_t *db,
> + const char *local_abspath,
> + const char *propname,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
- Julian