You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2012/11/28 20:25:50 UTC
Re: r1414123 - optimize iprops retrieval in libsvn_wc
> 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
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