You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2015/09/04 21:17:45 UTC
svn commit: r1701317 - in /subversion/trunk/subversion:
include/private/svn_ra_svn_private.h libsvn_ra_svn/marshal.c
Author: stefan2
Date: Fri Sep 4 19:17:44 2015
New Revision: 1701317
URL: http://svn.apache.org/r1701317
Log:
Finally, make svn_ra_svn__list_t actually a fully typed, ra_svn-specific
object. Update the creation functions; everything else already "just fits".
* subversion/include/private/svn_ra_svn_private.h
(svn_ra_svn__list_t): Define locally as a struct now.
(SVN_RA_SVN__LIST_ITEM): Update access macro.
* subversion/libsvn_ra_svn/marshal.c
(svn_ra_svn__to_private_array): Adapt conversion code.
(read_item): Efficiently create instances of the new array type.
Modified:
subversion/trunk/subversion/include/private/svn_ra_svn_private.h
subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Modified: subversion/trunk/subversion/include/private/svn_ra_svn_private.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_ra_svn_private.h?rev=1701317&r1=1701316&r2=1701317&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_ra_svn_private.h (original)
+++ subversion/trunk/subversion/include/private/svn_ra_svn_private.h Fri Sep 4 19:17:44 2015
@@ -34,13 +34,22 @@
extern "C" {
#endif /* __cplusplus */
+/** Memory representation of an on-the-wire data item. */
+typedef struct svn_ra_svn__item_t svn_ra_svn__item_t;
+
/* A list of svn_ra_svn__item_t objects. */
-typedef apr_array_header_t svn_ra_svn__list_t;
+typedef struct svn_ra_svn__list_t
+{
+ /* List contents (array). May be NULL if NELTS is 0. */
+ struct svn_ra_svn__item_t *items;
+
+ /* Number of elements in ITEMS. */
+ int nelts;
+} svn_ra_svn__list_t;
/* List element access macro. This is for transitional usage only.
* Once svn_ra_svn__list_t is finalized, this macro will become obsolete. */
-#define SVN_RA_SVN__LIST_ITEM(list, idx) \
- APR_ARRAY_IDX(list, idx, svn_ra_svn__item_t)
+#define SVN_RA_SVN__LIST_ITEM(list, idx) list->items[idx]
/** Memory representation of an on-the-wire data item. */
typedef struct svn_ra_svn__item_t
Modified: subversion/trunk/subversion/libsvn_ra_svn/marshal.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/marshal.c?rev=1701317&r1=1701316&r2=1701317&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_svn/marshal.c (original)
+++ subversion/trunk/subversion/libsvn_ra_svn/marshal.c Fri Sep 4 19:17:44 2015
@@ -153,13 +153,16 @@ svn_ra_svn__list_t *
svn_ra_svn__to_private_array(const apr_array_header_t *source,
apr_pool_t *result_pool)
{
- svn_ra_svn__list_t *result = apr_array_make(result_pool, source->nelts,
- sizeof(svn_ra_svn__item_t));
-
int i;
+
+ svn_ra_svn__list_t *result = apr_pcalloc(result_pool, sizeof(*result));
+ result->nelts = source->nelts;
+ result->items = apr_palloc(result_pool,
+ source->nelts * sizeof(*result->items));
+
for (i = 0; i < source->nelts; ++i)
{
- svn_ra_svn__item_t *sub_target = apr_array_push(result);
+ svn_ra_svn__item_t *sub_target = &result->items[i];
svn_ra_svn_item_t *sub_source = &APR_ARRAY_IDX(source, i,
svn_ra_svn_item_t);
@@ -1289,17 +1292,51 @@ static svn_error_t *read_item(svn_ra_svn
}
else if (c == '(')
{
+ /* Allow for up to 4 items in this list without re-allocation. */
+ svn_ra_svn__item_t stack_items[4];
+ svn_ra_svn__item_t *items = stack_items;
+ int capacity = sizeof(stack_items) / sizeof(stack_items[0]);
+ int count = 0;
+
/* Read in the list items. */
item->kind = SVN_RA_SVN_LIST;
- item->u.list = apr_array_make(pool, 4, sizeof(svn_ra_svn__item_t));
+ item->u.list = apr_pcalloc(pool, sizeof(*item->u.list));
+
while (1)
{
SVN_ERR(readbuf_getchar_skip_whitespace(conn, pool, &c));
if (c == ')')
break;
- listitem = apr_array_push(item->u.list);
+
+ /* Auto-expand the list. */
+ if (count == capacity)
+ {
+ svn_ra_svn__item_t *new_items
+ = apr_palloc(pool, 2 * capacity * sizeof(*new_items));
+ memcpy(new_items, items, capacity * sizeof(*new_items));
+ items = new_items;
+ capacity = 2 * capacity;
+ }
+
+ listitem = &items[count];
+ ++count;
+
SVN_ERR(read_item(conn, pool, listitem, c, level));
}
+
+ /* Store the list in ITEM - if not empty (= default). */
+ if (count)
+ {
+ item->u.list->nelts = count;
+
+ /* If we haven't allocated from POOL, yet, do it now. */
+ if (items == stack_items)
+ item->u.list->items = apr_pmemdup(pool, items,
+ count * sizeof(*items));
+ else
+ item->u.list->items = items;
+ }
+
SVN_ERR(readbuf_getchar(conn, pool, &c));
}
Re: svn commit: r1701317 - in /subversion/trunk/subversion:
include/private/svn_ra_svn_private.h libsvn_ra_svn/marshal.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sat, Sep 5, 2015 at 11:19 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 05.09.2015 18:53, Ivan Zhakov wrote:
> > On 5 September 2015 at 19:23, Branko Čibej <br...@wandisco.com> wrote:
> >> On 05.09.2015 02:53, Branko Čibej wrote:
> >>> On 04.09.2015 21:17, stefan2@apache.org wrote:
> >>>> Author: stefan2
> >>>> Date: Fri Sep 4 19:17:44 2015
> >>>> New Revision: 1701317
> >>>>
> >>>> URL: http://svn.apache.org/r1701317
> >>>> Log:
> >>>> Finally, make svn_ra_svn__list_t actually a fully typed,
> ra_svn-specific
> >>>> object. Update the creation functions; everything else already "just
> fits".
> >>> How is this code different from using APR arrays, except that the
> latter
> >>> needs a typecast on array item access? As far as I can see, you've
> >>> completely duplicated the APR array allocation strategy, including
> using
> >>> two allocations to create the array.
>
Not exactly: Quite some fraction of the arrays is empty
(20+%,greatly dependent on the command being run).
Those will only do a single allocation.
But you are right that this commit in and of itself does
not make much of a difference. With r1701318, however,
the average number of allocation drops below 1. This is
because re-allocations are extremely rare (1:2M in my
test case, YMMV). If a certain record has more than 4
sub-elements on the protocol side, it is easy to extend
the on-stack buffer size sufficiently to cover that. I'll go
over the *_parse_tuple calls to see where larger structs
are being used, so we get 0 resize except for unbounded
lists. In contrast to APR default array sizes, this will not
increase the amount of memory allocated from the pool.
As a bonus, the combination of r1701317,-8,-22 saves
some memory, slightly less than 100 bytes per array.
This is somewhat relevant for responses that contain
unbounded collections, e.g. a directory listing or a rev's
changed paths list.
> >>> The only significant difference is that capacity is being tracked
> >>> outside the svn_ra_svn__list_t structure during the construction of the
> >>> list.
>
There is also calling overhead for apr_array_push that
alone accounts for about 5% of the protocol processing
(no re-allocation taking place). It all comes down to lots
of piecemeal in a small section of code (one branch of
read_item).
> >>> Call me dense ... but can you please explain how exactly is this
> >>> better/faster than using APR arrays? (I'm not going to mention 'safer'
> >>> because it clearly isn't.) Code like this that is apparently meant be
> an
> >>> optimisation of something(?) really should have a bit of an explanatory
> >>> comment, IMO.
>
Sorry about that. I forgot to replicate the comment from
the original APR-array-based commit. After fixing that,
it now says the same things as I did above.
>> I played around with the apr_array_make implementation a bit and did
> >> some performance measurements with small array allocation and usage,
> >> with the following pattern:
> >>
> >> * in 60% of cases, the array does not get resized
>
And if that gets close to 100% the leverage becomes
even greater.
> >> * in 30% it gets resized once
> >> * in 10% it gets resized twice
> >>
> >> If I change apr_array_make to allocate the initial number of elements in
> >> the same block as the array header, I get a 15% speedup on this test
> >> case, compared to the default implementation. If I change it further to
> >> never set the element values to 0 in apr_array_make, I get an additional
> >> 10% speedup, for a total of 23%. So I'm guessing this is the number you
> >> were actually seeing.
>
Along those lines, yes. I'm getting about 20% for the
whole protocol handling (also see below) in a fully
optimized build. I have not tried to build APR with
similar settings and linking statically to it; I always
used the libs that came with the OS.
> > Is it speedup of overall Subversion over svn:// protocol operation or
> > just of APR array allocation?
>
> Stefan reported a 20% speedup of svn:// protocol processing. My test
> case doesn't measure that, of course.
>
In the case of 'svn diff --summary', protocol processing
is ~50% of the runtime, the remainder is UI processing.
Of that 50%, 20% were shaven off, i.e. a 10% saving
total in this case. If a client does something else with
the data, e.g. only print a total stat or filter it or store
parts of it in some data structure, the relative savings
could be higher.
The bottom line is that any longer response stream that
does not contain user file contents will see similar savings
in the protocol processing part. It is also fair to assume
that most clients won't do much more with the data than
our CL client does in the test case. That is list, log and
diff --summary should be faster now while having a lower
peak memory usage.
The underlying reason for the impact of array handling
in ra_svn is the structure of the protocol itself. Every C
struct and every optional element in it gets enclosed by
"( )", which become a "list" in the parser. More than 40%
of all items can be lists, or one list every 30 bytes or so.
Trunk is now able to send (server), receive and display
(client) such data at a sustained 1Gbps.
-- Stefan^2.
Re: svn commit: r1701317 - in /subversion/trunk/subversion:
include/private/svn_ra_svn_private.h libsvn_ra_svn/marshal.c
Posted by Branko Čibej <br...@wandisco.com>.
On 05.09.2015 18:53, Ivan Zhakov wrote:
> On 5 September 2015 at 19:23, Branko Čibej <br...@wandisco.com> wrote:
>> On 05.09.2015 02:53, Branko Čibej wrote:
>>> On 04.09.2015 21:17, stefan2@apache.org wrote:
>>>> Author: stefan2
>>>> Date: Fri Sep 4 19:17:44 2015
>>>> New Revision: 1701317
>>>>
>>>> URL: http://svn.apache.org/r1701317
>>>> Log:
>>>> Finally, make svn_ra_svn__list_t actually a fully typed, ra_svn-specific
>>>> object. Update the creation functions; everything else already "just fits".
>>> How is this code different from using APR arrays, except that the latter
>>> needs a typecast on array item access? As far as I can see, you've
>>> completely duplicated the APR array allocation strategy, including using
>>> two allocations to create the array.
>>>
>>> The only significant difference is that capacity is being tracked
>>> outside the svn_ra_svn__list_t structure during the construction of the
>>> list.
>>>
>>> Call me dense ... but can you please explain how exactly is this
>>> better/faster than using APR arrays? (I'm not going to mention 'safer'
>>> because it clearly isn't.) Code like this that is apparently meant be an
>>> optimisation of something(?) really should have a bit of an explanatory
>>> comment, IMO.
>> I played around with the apr_array_make implementation a bit and did
>> some performance measurements with small array allocation and usage,
>> with the following pattern:
>>
>> * in 60% of cases, the array does not get resized
>> * in 30% it gets resized once
>> * in 10% it gets resized twice
>>
>> If I change apr_array_make to allocate the initial number of elements in
>> the same block as the array header, I get a 15% speedup on this test
>> case, compared to the default implementation. If I change it further to
>> never set the element values to 0 in apr_array_make, I get an additional
>> 10% speedup, for a total of 23%. So I'm guessing this is the number you
>> were actually seeing.
> Is it speedup of overall Subversion over svn:// protocol operation or
> just of APR array allocation?
Stefan reported a 20% speedup of svn:// protocol processing. My test
case doesn't measure that, of course.
-- Brane
Re: svn commit: r1701317 - in /subversion/trunk/subversion:
include/private/svn_ra_svn_private.h libsvn_ra_svn/marshal.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 5 September 2015 at 19:23, Branko Čibej <br...@wandisco.com> wrote:
> On 05.09.2015 02:53, Branko Čibej wrote:
>> On 04.09.2015 21:17, stefan2@apache.org wrote:
>>> Author: stefan2
>>> Date: Fri Sep 4 19:17:44 2015
>>> New Revision: 1701317
>>>
>>> URL: http://svn.apache.org/r1701317
>>> Log:
>>> Finally, make svn_ra_svn__list_t actually a fully typed, ra_svn-specific
>>> object. Update the creation functions; everything else already "just fits".
>> How is this code different from using APR arrays, except that the latter
>> needs a typecast on array item access? As far as I can see, you've
>> completely duplicated the APR array allocation strategy, including using
>> two allocations to create the array.
>>
>> The only significant difference is that capacity is being tracked
>> outside the svn_ra_svn__list_t structure during the construction of the
>> list.
>>
>> Call me dense ... but can you please explain how exactly is this
>> better/faster than using APR arrays? (I'm not going to mention 'safer'
>> because it clearly isn't.) Code like this that is apparently meant be an
>> optimisation of something(?) really should have a bit of an explanatory
>> comment, IMO.
>
> I played around with the apr_array_make implementation a bit and did
> some performance measurements with small array allocation and usage,
> with the following pattern:
>
> * in 60% of cases, the array does not get resized
> * in 30% it gets resized once
> * in 10% it gets resized twice
>
> If I change apr_array_make to allocate the initial number of elements in
> the same block as the array header, I get a 15% speedup on this test
> case, compared to the default implementation. If I change it further to
> never set the element values to 0 in apr_array_make, I get an additional
> 10% speedup, for a total of 23%. So I'm guessing this is the number you
> were actually seeing.
Is it speedup of overall Subversion over svn:// protocol operation or
just of APR array allocation?
--
Ivan Zhakov
Re: svn commit: r1701317 - in /subversion/trunk/subversion:
include/private/svn_ra_svn_private.h libsvn_ra_svn/marshal.c
Posted by Branko Čibej <br...@wandisco.com>.
On 05.09.2015 02:53, Branko Čibej wrote:
> On 04.09.2015 21:17, stefan2@apache.org wrote:
>> Author: stefan2
>> Date: Fri Sep 4 19:17:44 2015
>> New Revision: 1701317
>>
>> URL: http://svn.apache.org/r1701317
>> Log:
>> Finally, make svn_ra_svn__list_t actually a fully typed, ra_svn-specific
>> object. Update the creation functions; everything else already "just fits".
> How is this code different from using APR arrays, except that the latter
> needs a typecast on array item access? As far as I can see, you've
> completely duplicated the APR array allocation strategy, including using
> two allocations to create the array.
>
> The only significant difference is that capacity is being tracked
> outside the svn_ra_svn__list_t structure during the construction of the
> list.
>
> Call me dense ... but can you please explain how exactly is this
> better/faster than using APR arrays? (I'm not going to mention 'safer'
> because it clearly isn't.) Code like this that is apparently meant be an
> optimisation of something(?) really should have a bit of an explanatory
> comment, IMO.
I played around with the apr_array_make implementation a bit and did
some performance measurements with small array allocation and usage,
with the following pattern:
* in 60% of cases, the array does not get resized
* in 30% it gets resized once
* in 10% it gets resized twice
If I change apr_array_make to allocate the initial number of elements in
the same block as the array header, I get a 15% speedup on this test
case, compared to the default implementation. If I change it further to
never set the element values to 0 in apr_array_make, I get an additional
10% speedup, for a total of 23%. So I'm guessing this is the number you
were actually seeing.
We can certainly change APR to get that extra 15% in, but we can't
remove the memset(0) because it's part of the existing API semantics.
However we can add (in apr-1.6 and 2.0) a new constructor, e.g.
apr_array_make_uninitialized, that would not clear the element values.
This wouldn't help with apr_array_push, but note that there's already a
private function called apr_array_push_noclear that's used with apr_table_t
Obviously I'd much rather make these enhancements in APR than have yet
another custom allocation hack in Subversion.
-- Brane
Re: svn commit: r1701317 - in /subversion/trunk/subversion:
include/private/svn_ra_svn_private.h libsvn_ra_svn/marshal.c
Posted by Branko Čibej <br...@wandisco.com>.
On 04.09.2015 21:17, stefan2@apache.org wrote:
> Author: stefan2
> Date: Fri Sep 4 19:17:44 2015
> New Revision: 1701317
>
> URL: http://svn.apache.org/r1701317
> Log:
> Finally, make svn_ra_svn__list_t actually a fully typed, ra_svn-specific
> object. Update the creation functions; everything else already "just fits".
How is this code different from using APR arrays, except that the latter
needs a typecast on array item access? As far as I can see, you've
completely duplicated the APR array allocation strategy, including using
two allocations to create the array.
The only significant difference is that capacity is being tracked
outside the svn_ra_svn__list_t structure during the construction of the
list.
Call me dense ... but can you please explain how exactly is this
better/faster than using APR arrays? (I'm not going to mention 'safer'
because it clearly isn't.) Code like this that is apparently meant be an
optimisation of something(?) really should have a bit of an explanatory
comment, IMO.
-- Brane