On Fri, Feb 21, 2014 at 4:55 PM, Julian Foad <julianfoad_at_btopenworld.com>wrote:
> Stefan,
>
> In svn.apache.org/r1395434 and svn.apache.org/r1483310, you optimised
> svn_rangelist_dup().
>
> An svn_rangelist_t is an APR array of pointers to small simple objects.
>
> I guessed we would have other places where we want to duplicate an
> array of pointers to simple objects, but I guessed wrong and
> can't find any. Nevertheless, how about we factor out the generic
> array-duplication code like this, in order to maintain a clear separation
> between rangelist code and generic array code? I think that makes it easier
> for a reader to see that there's nothing special happening there that
> concerns rangelists specifically.
>
Yes, that makes sense.
> I have at the back of my mind that this function could very well be
> proposed for inclusion in APR, but I am not presently thinking of doing so.
>
It is still a rather specific application (copy depth == 1).
APR would probably want something with copy callbacks,
which would defeat the purpose of the whole exercise.
> Of course, parameterizing the 'object size' like this may make it slightly
> slower. I don't imagine it would be significant but haven't measured it.
> Does that bother you in this case?
>
I looked at the generated code and it is basically identical
to what we get without the patch - thanks to the constant
propagation in higher optimization levels.
[[[
> Index: subversion/libsvn_subr/mergeinfo.c
> ===================================================================
> --- subversion/libsvn_subr/mergeinfo.c (revision 1570152)
> +++ subversion/libsvn_subr/mergeinfo.c (working copy)
> @@ -2237,35 +2237,48 @@ svn_mergeinfo__add_suffix_to_mergeinfo(s
> rangelist);
> }
>
> return SVN_NO_ERROR;
> }
>
> -svn_rangelist_t *
> -svn_rangelist_dup(const svn_rangelist_t *rangelist, apr_pool_t *pool)
> +/* Deep-copy an array of pointers to simple objects.
> + *
> + * Return a duplicate in POOL of the array ARRAY of pointers to objects
> + * of size OBJECT_SIZE bytes. Duplicate each object bytewise.
> + */
> +static apr_array_header_t *
> +ptr_array_dup(const apr_array_header_t *array,
> + size_t object_size,
> + apr_pool_t *pool)
> {
> - svn_rangelist_t *new_rl = apr_array_make(pool, rangelist->nelts,
> - sizeof(svn_merge_range_t *));
> + apr_array_header_t *new_array = apr_array_make(pool, array->nelts,
> + sizeof(void *));
>
> /* allocate target range buffer with a single operation */
> - svn_merge_range_t *copy = apr_palloc(pool, sizeof(*copy) *
> rangelist->nelts);
> + char *copy = apr_palloc(pool, object_size * array->nelts);
>
> /* for efficiency, directly address source and target reference buffers
> */
> - svn_merge_range_t **source = (svn_merge_range_t **)(rangelist->elts);
> - svn_merge_range_t **target = (svn_merge_range_t **)(new_rl->elts);
> + void **source = (void **)(array->elts);
> + void **target = (void **)(new_array->elts);
> int i;
>
> /* copy ranges iteratively and link them into the target range list */
> - for (i = 0; i < rangelist->nelts; i++)
> + for (i = 0; i < array->nelts; i++)
> {
> - copy[i] = *source[i];
> - target[i] = ©[i];
> + target[i] = ©[i * object_size];
> + memcpy(target[i], source[i], object_size);
> }
> - new_rl->nelts = rangelist->nelts;
> + new_array->nelts = array->nelts;
> +
> + return new_array;
> +}
>
> - return new_rl;
> +svn_rangelist_t *
> +svn_rangelist_dup(const svn_rangelist_t *rangelist, apr_pool_t *pool)
> +{
> + return ptr_array_dup(rangelist, sizeof(svn_merge_range_t), pool);
> }
>
> svn_merge_range_t *
> svn_merge_range_dup(const svn_merge_range_t *range, apr_pool_t *pool)
> {
> svn_merge_range_t *new_range = apr_palloc(pool, sizeof(*new_range));
> ]]]
>
Committed as r1570904.
-- Stefan^2.
Received on 2014-02-22 21:28:18 CET