You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2014/08/26 18:46:54 UTC
Re: svn commit: r1618641 - /subversion/trunk/subversion/libsvn_fs_fs/id.c
On 18 August 2014 19:50, <st...@apache.org> wrote:
> Author: stefan2
> Date: Mon Aug 18 15:50:44 2014
> New Revision: 1618641
>
> URL: http://svn.apache.org/r1618641
> Log:
> Within FSFS, replace the use of svn__strtol with a variant that
> provides overflow detection. FSFS needs a locale-independent
> and preferrably quick number parser.
>
[...]
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/id.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/id.c Mon Aug 18 15:50:44 2014
> @@ -48,12 +48,55 @@ typedef struct fs_fs__id_t
>
>
>
> +/** Like strtol but with a fixed base of 10, locale independent and limited
> + * to non-negative values. Overflows are indicated by a FALSE return value
> + * in which case *RESULT_P will not be modified.
> + *
> + * This allows the compiler to generate massively faster code.
> + * (E.g. Avoiding locale specific processing). ID parsing is one of the
> + * most CPU consuming parts of FSFS data access. Better be quick.
> + */
> +static svn_boolean_t
> +fast__strtol(long *result_p,
> + const char* buffer,
> + const char** end)
> +{
> + /* We allow positive values only. Unsigned arithmetics tend to be faster
> + * as they allow for a wider range of compiler-side optimizations. */
> + unsigned long result = 0;
> + while (1)
> + {
> + unsigned long c = (unsigned char)*buffer - (unsigned char)'0';
> + unsigned long next;
> +
> + /* This implies the NUL check. */
> + if (c > 9)
> + break;
> +
> + next = result * 10 + c;
> +
> + /* Overflow check. */
> + if (next < result)
> + return FALSE;
> +
> + result = next;
> + ++buffer;
> + }
> +
> + *end = buffer;
> + *result_p = (long)result;
> +
> + return TRUE;
> +}
> +
Stefan,
It maybe worth to make tests for this function (making it private to
libsvn_subr for this). What do you think?
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1618641 - /subversion/trunk/subversion/libsvn_fs_fs/id.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Aug 27, 2014 at 6:58 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 27 August 2014 19:41, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> > On Wed, Aug 27, 2014 at 9:13 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 26 August 2014 21:18, Branko Čibej <br...@wandisco.com> wrote:
> >> > On 26.08.2014 18:46, Ivan Zhakov wrote:
> >> >
> >> > On 18 August 2014 19:50, <st...@apache.org> wrote:
> >> >
> >> > Author: stefan2
> >> > Date: Mon Aug 18 15:50:44 2014
> >> > New Revision: 1618641
> >> >
> >> > URL: http://svn.apache.org/r1618641
> >> > Log:
> >> > Within FSFS, replace the use of svn__strtol with a variant that
> >> > provides overflow detection. FSFS needs a locale-independent
> >> > and preferrably quick number parser.
> >> >
> >> > [...]
> >> >
> >> > Stefan,
> >> >
> >> > It maybe worth to make tests for this function (making it private to
> >> > libsvn_subr for this). What do you think?
> >> >
> >> >
> >> > Currently, this function is only used in a couple places within FSFS.
> >> > FWIW
> >> > this does not prevent writing tests for it; we do already have C tests
> >> > for
> >> > libsvn_fs_fs.
> >> >
> >> > I did raise the same question in Sheffield, but was convinced that
> >> > moving
> >> > that code around for formality's sake serves no useful purpose.
> >> >
> >> I'm fine to keep libsvn_fs_fs if we could write test for this
> >> function, but I think that test will be very useful for this function.
> >
> >
> > Test added in r1620913.
> >
> Is it true that you uncovered the bug in svn_fs_fs__id_parse() fixed
> in r1620912 using this new test?
>
Not in that sense. While verifying that the new test covers
all overflow detection paths, I saw and opportunity to make
the parser more strict that it has ever been in previous
releases. That has kind of been the point of making all
ID elements proper numbers instead of converting them
from and to arbitrary strings.
-- Stefan^2.
Re: svn commit: r1618641 - /subversion/trunk/subversion/libsvn_fs_fs/id.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 27 August 2014 19:41, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Wed, Aug 27, 2014 at 9:13 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 26 August 2014 21:18, Branko Čibej <br...@wandisco.com> wrote:
>> > On 26.08.2014 18:46, Ivan Zhakov wrote:
>> >
>> > On 18 August 2014 19:50, <st...@apache.org> wrote:
>> >
>> > Author: stefan2
>> > Date: Mon Aug 18 15:50:44 2014
>> > New Revision: 1618641
>> >
>> > URL: http://svn.apache.org/r1618641
>> > Log:
>> > Within FSFS, replace the use of svn__strtol with a variant that
>> > provides overflow detection. FSFS needs a locale-independent
>> > and preferrably quick number parser.
>> >
>> > [...]
>> >
>> > Stefan,
>> >
>> > It maybe worth to make tests for this function (making it private to
>> > libsvn_subr for this). What do you think?
>> >
>> >
>> > Currently, this function is only used in a couple places within FSFS.
>> > FWIW
>> > this does not prevent writing tests for it; we do already have C tests
>> > for
>> > libsvn_fs_fs.
>> >
>> > I did raise the same question in Sheffield, but was convinced that
>> > moving
>> > that code around for formality's sake serves no useful purpose.
>> >
>> I'm fine to keep libsvn_fs_fs if we could write test for this
>> function, but I think that test will be very useful for this function.
>
>
> Test added in r1620913.
>
Is it true that you uncovered the bug in svn_fs_fs__id_parse() fixed
in r1620912 using this new test?
--
Ivan Zhakov
Re: svn commit: r1618641 - /subversion/trunk/subversion/libsvn_fs_fs/id.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Aug 27, 2014 at 9:13 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 26 August 2014 21:18, Branko Čibej <br...@wandisco.com> wrote:
> > On 26.08.2014 18:46, Ivan Zhakov wrote:
> >
> > On 18 August 2014 19:50, <st...@apache.org> wrote:
> >
> > Author: stefan2
> > Date: Mon Aug 18 15:50:44 2014
> > New Revision: 1618641
> >
> > URL: http://svn.apache.org/r1618641
> > Log:
> > Within FSFS, replace the use of svn__strtol with a variant that
> > provides overflow detection. FSFS needs a locale-independent
> > and preferrably quick number parser.
> >
> > [...]
> >
> > Stefan,
> >
> > It maybe worth to make tests for this function (making it private to
> > libsvn_subr for this). What do you think?
> >
> >
> > Currently, this function is only used in a couple places within FSFS.
> FWIW
> > this does not prevent writing tests for it; we do already have C tests
> for
> > libsvn_fs_fs.
> >
> > I did raise the same question in Sheffield, but was convinced that moving
> > that code around for formality's sake serves no useful purpose.
> >
> I'm fine to keep libsvn_fs_fs if we could write test for this
> function, but I think that test will be very useful for this function.
>
Test added in r1620913.
-- Stefan^2.
Re: svn commit: r1618641 - /subversion/trunk/subversion/libsvn_fs_fs/id.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 26 August 2014 21:18, Branko Čibej <br...@wandisco.com> wrote:
> On 26.08.2014 18:46, Ivan Zhakov wrote:
>
> On 18 August 2014 19:50, <st...@apache.org> wrote:
>
> Author: stefan2
> Date: Mon Aug 18 15:50:44 2014
> New Revision: 1618641
>
> URL: http://svn.apache.org/r1618641
> Log:
> Within FSFS, replace the use of svn__strtol with a variant that
> provides overflow detection. FSFS needs a locale-independent
> and preferrably quick number parser.
>
> [...]
>
> Stefan,
>
> It maybe worth to make tests for this function (making it private to
> libsvn_subr for this). What do you think?
>
>
> Currently, this function is only used in a couple places within FSFS. FWIW
> this does not prevent writing tests for it; we do already have C tests for
> libsvn_fs_fs.
>
> I did raise the same question in Sheffield, but was convinced that moving
> that code around for formality's sake serves no useful purpose.
>
I'm fine to keep libsvn_fs_fs if we could write test for this
function, but I think that test will be very useful for this function.
--
Ivan Zhakov
Re: svn commit: r1618641 - /subversion/trunk/subversion/libsvn_fs_fs/id.c
Posted by Branko Čibej <br...@wandisco.com>.
On 26.08.2014 18:46, Ivan Zhakov wrote:
> On 18 August 2014 19:50, <st...@apache.org> wrote:
>> Author: stefan2
>> Date: Mon Aug 18 15:50:44 2014
>> New Revision: 1618641
>>
>> URL: http://svn.apache.org/r1618641
>> Log:
>> Within FSFS, replace the use of svn__strtol with a variant that
>> provides overflow detection. FSFS needs a locale-independent
>> and preferrably quick number parser.
>>
> [...]
>
> Stefan,
>
> It maybe worth to make tests for this function (making it private to
> libsvn_subr for this). What do you think?
Currently, this function is only used in a couple places within FSFS.
FWIW this does not prevent writing tests for it; we do already have C
tests for libsvn_fs_fs.
I did raise the same question in Sheffield, but was convinced that
moving that code around for formality's sake serves no useful purpose.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com