[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH v2] Saving a few cycles, part 3/3

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Wed, 05 May 2010 12:02:48 +0100

On Sun, 2010-05-02, Stefan Fuhrmann wrote:
> Bert Huijben wrote:
> >> Stefan Fuhrmann wrote:
> >>
> >> * subversion/include/svn_delta.h
> >> (enum svn_delta_action): ensure that these definitions
> >> will always have these values, even if new definitions
> >> were added
> >
> > This part is not necessary. These values are part of our public API
> > compatibility promise and that makes the values static for 1.X.
> >
> > The C compiler defines the same values... and if it didn't you are not
> > allowed to change the values (for the same binary compatibility reasons).
> >
> Correct. I just wanted these values to become more visible
> to anyone who might want to extend the enumeration.
>
> But I'm fine with removing that part. If the encoding should
> be changed accidentally, virtually any repository read access
> will fail with a checksum error. So, it would not linger there
> unnoticed. So, please remove that part if you apply the patch.

I compromised here by just adding a note that the svndiff implementation
relies on these values.

> >> (copy_source_ops): dito; optimize a common case
> >>
> >> * subversion/libsvn_delta/svndiff.c
> >> (decode_file_offset, decode_size): use slightly more
> >> efficient formulations
> >> (decode_instruction): directly map action codes -
> >> made possible by defining the enum values
> >
> > The enum values were already defined so that doesn't change anything (see
> > above: they can't change anyway).
> >
> O.k. omit the "- made possible ..." part.

Done.

> > Optimizing a % operator for values of 1 and 2 looks like code obfuscation to
> > me, unless you can show me some numbers that it actually makes a difference.
> > I would assume that the processor optimizes that case itself.
> >
> The performance advantage is very small anyway - no
> reason for me to put any more effort in it. Just leave it
> out from the commit.

OK, left out.

Stefan, thanks for persisting and answering all our questions.

This looks good now. I'm committing it.

Committed revision 941243.

- Julian
Received on 2010-05-05 13:03:26 CEST

This is an archived mail posted to the Subversion Dev mailing list.