Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Subject: Re: svn commit: r1701317 - in /subversion/trunk/subversion:
 include/private/svn_ra_svn_private.h libsvn_ra_svn/marshal.c
To: dev@subversion.apache.org
References: <20150904191745.534A2AC0232@hades.apache.org>
 <55EA3D0E.3000009@wandisco.com>
From: =?UTF-8?Q?Branko_=c4=8cibej?= <brane@wandisco.com>
Organization: WANdisco
Message-ID: <55EB1711.9070809@wandisco.com>
Date: Sat, 5 Sep 2015 18:23:45 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:38.0)
 Gecko/20100101 Thunderbird/38.2.0
MIME-Version: 1.0
In-Reply-To: <55EA3D0E.3000009@wandisco.com>
Content-Type: multipart/mixed;
 boundary="------------070402030801090102070202"

This is a multi-part message in MIME format.
--------------070402030801090102070202
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

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


--------------070402030801090102070202
Content-Type: text/plain; charset=UTF-8; x-mac-type="0"; x-mac-creator="0";
 name="test-array-alloc.c"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="test-array-alloc.c"

I2luY2x1ZGUgPHN0ZGxpYi5oPgoKI2luY2x1ZGUgImFwcl9wb29scy5oIgojaW5jbHVkZSAi
YXByX3RhYmxlcy5oIgoKc3RhdGljIHZvaWQKdGVzdF9hcnJheV9hbGxvYyhhcHJfcG9vbF90
ICpwb29sKQp7CiAgICBpbnQgaSwgaiwgazsKICAgIGZvciAoaSA9IDA7IGkgPCA1MDAwMDsg
KytpKQogICAgewogICAgICAgIGFwcl9wb29sX2NsZWFyKHBvb2wpOwogICAgICAgIGZvciAo
aiA9IDA7IGogPCAxMDAwMDsgKytqKQogICAgICAgIHsKICAgICAgICAgICAgY29uc3QgaW50
IG5lbHRzID0gKChqICUgMTAgPT0gMCkgPyAxMQogICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgOiAoaiAlIDEwIDwgNykgPyAzIDogNyk7CiAgICAgICAgICAgIGFwcl9hcnJheV9o
ZWFkZXJfdCAqYSA9IGFwcl9hcnJheV9tYWtlKHBvb2wsIDQsIDEpOwogICAgICAgICAgICBm
b3IgKGsgPSAwOyBrIDwgbmVsdHM7ICsraykKICAgICAgICAgICAgICAgIEFQUl9BUlJBWV9Q
VVNIKGEsIGNoYXIpID0gJ1wwJzsKICAgICAgICB9CiAgICB9Cn0KCmludCBtYWluKHZvaWQp
CnsKICAgIGFwcl9wb29sX3QgKnBvb2wgPSBOVUxMOwoKICAgIGlmIChhcHJfaW5pdGlhbGl6
ZSgpKQogICAgICAgIGV4aXQoMSk7CiAgICBhdGV4aXQoYXByX3Rlcm1pbmF0ZSk7CgogICAg
YXByX3Bvb2xfY3JlYXRlKCZwb29sLCBOVUxMKTsKICAgIHRlc3RfYXJyYXlfYWxsb2MocG9v
bCk7CiAgICByZXR1cm4gMDsKfQo=
--------------070402030801090102070202--