Return-Path: X-Original-To: apmail-subversion-dev-archive@minotaur.apache.org Delivered-To: apmail-subversion-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id EE4E5174B8 for ; Wed, 29 Oct 2014 11:22:22 +0000 (UTC) Received: (qmail 37683 invoked by uid 500); 29 Oct 2014 11:22:17 -0000 Delivered-To: apmail-subversion-dev-archive@subversion.apache.org Received: (qmail 37627 invoked by uid 500); 29 Oct 2014 11:22:17 -0000 Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@subversion.apache.org Received: (qmail 37614 invoked by uid 99); 29 Oct 2014 11:22:17 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 29 Oct 2014 11:22:16 +0000 X-ASF-Spam-Status: No, hits=1.5 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of stefan.fuhrmann@wandisco.com designates 209.85.223.181 as permitted sender) Received: from [209.85.223.181] (HELO mail-ie0-f181.google.com) (209.85.223.181) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 29 Oct 2014 11:21:51 +0000 Received: by mail-ie0-f181.google.com with SMTP id rp18so986948iec.26 for ; Wed, 29 Oct 2014 04:21:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=wandisco.com; s=gapps; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=beIb099P/t666+okA5YAO6YsbGujkkMohBdFqv2o/Gk=; b=Q1v3aH5TR37AaJX5/I1j2af2kWsErfVSn+EWw+HtSuPyCwdRxHeisNrEONOSlaOnkG YWKzYAycxot1NAVfoq1V6a506NHZD1MkxWYXEBPfmCDOjyIFOyz9lqEzjg6eTCJi4ftw ckQ/ThFWWH2iMdXgpfuqmo/z0N+vRT+FSfFZQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=beIb099P/t666+okA5YAO6YsbGujkkMohBdFqv2o/Gk=; b=CqCum/92D9/fpfE+xvD2VQhTBFBZS8QT7eJcKNW73SIKr+Oo7ZypHjoSNWTWxYbIiT U/vE9ffWwAQYZRYzRvHxzYSkbndpbk8rLCCikRAx2k+THabF3ctVwES5IUSicCb252zz HctHxtHMPW0vgZlMZPGunYVVnkOJlf+KeWGx7pA8ALC37XGFK96BB0apCufdAE/hOTZf lBVwgY44NcGOdWoOB/9M3IYXP0nCn73IDUWOiR6XQB+22EKaKJ07PU4gwSlqulnCWqH/ WnRcZSlrTxZDql4vWqRfPvLjUV9tQulca2N+1kskLHsJE2pYOscYl5S+Hgdmu443tdUg woYA== X-Gm-Message-State: ALoCoQmhXYrsGs19sUPS/OpIg2S9Pod20L+9EGK9ZdrxPBli9ae8/r4oMCgzmDlFHY5p9/bFordr MIME-Version: 1.0 X-Received: by 10.43.170.134 with SMTP id nq6mr9894787icc.30.1414581709304; Wed, 29 Oct 2014 04:21:49 -0700 (PDT) Received: by 10.50.40.226 with HTTP; Wed, 29 Oct 2014 04:21:49 -0700 (PDT) In-Reply-To: <5450A909.3070400@wandisco.com> References: <20141028131930.B841023888A6@eris.apache.org> <5450A909.3070400@wandisco.com> Date: Wed, 29 Oct 2014 12:21:49 +0100 Message-ID: Subject: Re: svn commit: r1634875 - /subversion/trunk/subversion/libsvn_fs_fs/revprops.c From: Stefan Fuhrmann To: =?UTF-8?Q?Branko_=C4=8Cibej?= Cc: Subversion Development Content-Type: multipart/alternative; boundary=001a11c2e2224a0ed605068df8b9 X-Virus-Checked: Checked by ClamAV on apache.org --001a11c2e2224a0ed605068df8b9 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Wed, Oct 29, 2014 at 9:44 AM, Branko =C4=8Cibej wro= te: > On 28.10.2014 14:19, stefan2@apache.org wrote: > > Author: stefan2 > > Date: Tue Oct 28 13:19:30 2014 > > New Revision: 1634875 > > > > URL: http://svn.apache.org/r1634875 > > Log: > > Speed up packed revprop access by tuning the manifest file parser. > > [...] > > > +/* Return the minimum length of any packed revprop file name in > REVPROPS. */ > > +static apr_size_t > > +get_min_filename_len(packed_revprops_t *revprops) > > +{ > > + char number_buffer[SVN_INT64_BUFFER_SIZE]; > > + > > + /* The revprop filenames have the format . - with > being > > + * at least the first rev in the shard and having at least o= ne > > + * digit. Thus, the minimum is 2 + #decimal places in the start rev= . > > + */ > > + return svn__i64toa(number_buffer, revprops->manifest_start) + 2; > > +} > > Are you absolutely sure this is correct? According to the comment, you > should be returning > > strlen(svn_i64toa(...)) + 2 > svn_i64toa returns the number of non-NUL chars written into the buffer provided by the caller. There is no memory allocation. An alternative sequence would look like this: svn_i64toa(buf, val); return strlen(buf)+2; > As it is, you end up allocating buffers that are orders of magnitude > larger than necessary. > The value returned by get_min_filename_len is not used to allocate some buffer but to skip a number of chars in the input buffer during parsing. Correctness is checked afterwards by comparing the number of file names parsed with the number of revisions in the shard; they must match. So, using a value that is too large here is certain to make the parser error out. -- Stefan^2. --001a11c2e2224a0ed605068df8b9 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Wed, Oct 29, 2014 at 9:44 AM, Branko =C4=8Cibej &l= t;brane@wandisco.co= m> wrote:
stefan2@apache= .org wrote:
> Author: stefan2
> Date: Tue Oct 28 13:19:30 2014
> New Revision: 1634875
>
> URL: http= ://svn.apache.org/r1634875
> Log:
> Speed up packed revprop access by tuning the manifest file parser.

[...]

> +/* Return the minimum length of any packed revprop file name in REVPR= OPS. */
> +static apr_size_t
> +get_min_filename_len(packed_revprops_t *revprops)
> +{
> +=C2=A0 char number_buffer[SVN_INT64_BUFFER_SIZE];
> +
> +=C2=A0 /* The revprop filenames have the format <REV>.<COUNT= > - with <REV> being
> +=C2=A0 =C2=A0* at least the first rev in the shard and <COUNT> = having at least one
> +=C2=A0 =C2=A0* digit.=C2=A0 Thus, the minimum is 2 + #decimal places = in the start rev.
> +=C2=A0 =C2=A0*/
> +=C2=A0 return svn__i64toa(number_buffer, revprops->manifest_start)= + 2;
> +}

Are you absolutely sure this is correct? According to the comment, you
should be returning

=C2=A0 =C2=A0 strlen(svn_i64toa(...)) + 2

svn_i64t= oa returns the number of non-NUL chars written into the
buffer provided = by the caller. There is no memory allocation. An
alternative sequence wo= uld look like this:

svn_i64toa(buf, val);
return strle= n(buf)+2;
=C2=A0
As it is, you end up allocating buffers that are orders of magnitude
larger than necessary.

The value returned by get_= min_filename_len is not used to allocate
some buffer but to skip a numbe= r of chars in the input buffer during
p= arsing. Correctness is checked afterwards by comparing the number
=
of file names parsed with the number of revision= s in the shard; they
must match. So, using a value that is too large her= e is certain to
make the parser error out.

-- Stefan^2.

--001a11c2e2224a0ed605068df8b9--