You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Fuhrmann <st...@alice-dsl.de> on 2010/05/01 17:02:53 UTC
[PATCH v2] Saving a few cycles, part 3/3
Hi devs,
I reworked the patch set according to the feedback I got
here on this list. This is what changed in this last part:
* add rationales to the commentary
* bring svndiff.c changes closer to the original code
* use svn_ctype_* functions in checksum parser
-- Stefan^2.
[[[
Various local optimizations. These opportunities became
visible after significantly optimizing other parts of svn. The
total savings for a 'svn export' is almost 3 percent on top
of the previous.
The largest savings can be attributed to the svndiff.c
changes (~1.5%) and the checksum parser (~1%).
* subversion/include/svn_delta.h
(enum svn_delta_action): ensure that these definitions
will always have these values, even if new definitions
were added
* subversion/libsvn_delta/compose_delta.c
(search_offset_index): use size_t to index memory.
This is mainly a consistency fix but may actually result
in slightly higher performance due to fewer conversions.
(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
* subversion/libsvn_subr/checksum.c
(svn_checksum_parse_hex): eliminate calls to locale-
aware CRT functions. At least with MS compilers,
these are very expensive.
* subversion/libsvn_subr/stream.c
(stream_readline): numbytes is invariant until EOF
patch by stefanfuhrmann < at > alice-dsl.de
]]]
Re: [PATCH v2] Saving a few cycles, part 3/3
Posted by Stefan Sperling <st...@elego.de>.
On Tue, May 11, 2010 at 07:43:33AM -0400, Mark Phippard wrote:
> On Tue, May 11, 2010 at 7:27 AM, Stefan Sperling <st...@elego.de> wrote:
> > On Tue, May 11, 2010 at 01:36:26AM +0200, Johan Corveleyn wrote:
> >> As I understand your set of patches, you're mainly focusing on saving
> >> cpu cycles, and not on avoiding I/O where possible (unless I'm missing
> >> something). Maybe some of the low- or high-level algorithms in the
> >> back-end can be reworked a bit to reduce the amount of I/O? Or maybe
> >> some clever caching can avoid some file accesses?
> >
> > In general, I think trying to work around I/O slowness by loading
> > stuff into RAM (caching) is a bad idea. You're just taking away memory
> > from the OS buffer cache if you do this. A good buffer cache in the OS
> > should make open/close/seek fast. (So don't run a windows server if
> > you can avoid it.)
> >
> > The only point where it's worth thinking about optimizing I/O
> > access is when you get to clustered, distributed storage, because
> > at that point every I/O request translated into a network packet.
>
> You had me until that last part. I think we should ALWAYS be thinking
> about optimizing I/O. I have little doubt that is where the biggest
> performance bottlenecks live (other than network of course). I agree
> that making a big cache is probably not the best way to go, but I
> think we should always be looking for optimizations where we avoid
> repeated open/closes that are not necessary.
That's true. Avoiding repeated open/close of the same file
is a good optimisation. Even with a good buffer cache it will
make a difference.
So s/The only point/One point/ :)
> I think it is extremely common that our customers have their
> repositories on NFS-mounted or SAN storage. While these often have
> fast disk subsystems there is still a noticeable penalty for file
> opens. Have you looked at Blair's wiki before?
>
> http://www.orcaware.com/svn/wiki/Server_performance_tuning_for_Linux_and_Unix
Thanks, that was an interesting read.
Of course, network filesystems like NFS have the same network
overhead penalty (except that caching on the local client is
probably a bit easier than with truly distributed storage,
but that's a minor detail).
Stefan
Re: [PATCH v2] Saving a few cycles, part 3/3
Posted by Mark Phippard <ma...@gmail.com>.
On Tue, May 11, 2010 at 7:27 AM, Stefan Sperling <st...@elego.de> wrote:
> On Tue, May 11, 2010 at 01:36:26AM +0200, Johan Corveleyn wrote:
>> As I understand your set of patches, you're mainly focusing on saving
>> cpu cycles, and not on avoiding I/O where possible (unless I'm missing
>> something). Maybe some of the low- or high-level algorithms in the
>> back-end can be reworked a bit to reduce the amount of I/O? Or maybe
>> some clever caching can avoid some file accesses?
>
> In general, I think trying to work around I/O slowness by loading
> stuff into RAM (caching) is a bad idea. You're just taking away memory
> from the OS buffer cache if you do this. A good buffer cache in the OS
> should make open/close/seek fast. (So don't run a windows server if
> you can avoid it.)
>
> The only point where it's worth thinking about optimizing I/O
> access is when you get to clustered, distributed storage, because
> at that point every I/O request translated into a network packet.
You had me until that last part. I think we should ALWAYS be thinking
about optimizing I/O. I have little doubt that is where the biggest
performance bottlenecks live (other than network of course). I agree
that making a big cache is probably not the best way to go, but I
think we should always be looking for optimizations where we avoid
repeated open/closes that are not necessary.
I think it is extremely common that our customers have their
repositories on NFS-mounted or SAN storage. While these often have
fast disk subsystems there is still a noticeable penalty for file
opens. Have you looked at Blair's wiki before?
http://www.orcaware.com/svn/wiki/Server_performance_tuning_for_Linux_and_Unix
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: [PATCH v2] Saving a few cycles, part 3/3
Posted by Stefan Sperling <st...@elego.de>.
On Tue, May 11, 2010 at 01:36:26AM +0200, Johan Corveleyn wrote:
> As I understand your set of patches, you're mainly focusing on saving
> cpu cycles, and not on avoiding I/O where possible (unless I'm missing
> something). Maybe some of the low- or high-level algorithms in the
> back-end can be reworked a bit to reduce the amount of I/O? Or maybe
> some clever caching can avoid some file accesses?
In general, I think trying to work around I/O slowness by loading
stuff into RAM (caching) is a bad idea. You're just taking away memory
from the OS buffer cache if you do this. A good buffer cache in the OS
should make open/close/seek fast. (So don't run a windows server if
you can avoid it.)
The only point where it's worth thinking about optimizing I/O
access is when you get to clustered, distributed storage, because
at that point every I/O request translated into a network packet.
Stefan
Re: [PATCH v2] Saving a few cycles, part 3/3
Posted by Johan Corveleyn <jc...@gmail.com>.
On Sun, May 9, 2010 at 11:25 PM, Stefan Fuhrmann
<st...@alice-dsl.de> wrote:
[...]
>
> Thanks for committing! I just got the last patch set out
> (zlib) and will start detailed performance measurements.
> Hopefully, all my other patches will then get committed
> as well ;)
>
> Currently, I get 2.6s (real) for an svn export -q of
> svn://localhost/apache/subversion/trunk and 1.6s (real)
> if I disable compression for the data transfer.
I'm not an expert (especially not in optimizing C code), but I'm
interested in all bits and pieces that can improve svn's performance.
I'm just wondering whether you're focusing on the biggest bottlenecks,
when looking at the back-end.
In my experience, FSFS' biggest bottleneck is disk I/O (at least when
we're looking at large read operations, like "log" and "annotate";
don't know for update/checkout/export). For instance "svn log" opens
and closes each interesting rev file numerous times (up to 10 times)
and reads them through several times (see e.g. [1], [2] and [3]).
Packed rev files don't help a lot, because the packed file is
opened/closed and seeked through just as many times.
As I understand your set of patches, you're mainly focusing on saving
cpu cycles, and not on avoiding I/O where possible (unless I'm missing
something). Maybe some of the low- or high-level algorithms in the
back-end can be reworked a bit to reduce the amount of I/O? Or maybe
some clever caching can avoid some file accesses?
Just to be clear: I think it's great that someone is looking at
optimizing things. I'm just wondering whether even bigger gains are
possible (especially regarding log and annotate, but maybe the same is
relevant for update/checkout/export). And it's not that I can help a
lot, I'm mostly an innocent observer here :-).
[1] http://svn.haxx.se/dev/archive-2009-06/0459.shtml
[2] http://svn.haxx.se/dev/archive-2007-08/0239.shtml
[3] I've also done some "poor man's profiling" for this once, by
adding printf's to APR's apr_file_open and file_cleanup (which is
called by apr_file_close, but also when a file is closed because its
pool is cleaned up). It was quite interesting to see, and to correlate
this with the higher level flow. But I had to move on to other
priorities, so couldn't follow through on it ...
Cheers
--
Johan
Re: [PATCH v2] Saving a few cycles, part 3/3
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
Julian Foad wrote:
> 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.
>
Thanks for committing! I just got the last patch set out
(zlib) and will start detailed performance measurements.
Hopefully, all my other patches will then get committed
as well ;)
Currently, I get 2.6s (real) for an svn export -q of
svn://localhost/apache/subversion/trunk and 1.6s (real)
if I disable compression for the data transfer.
-- Stefan^2.
Re: [PATCH v2] Saving a few cycles, part 3/3
Posted by Julian Foad <ju...@wandisco.com>.
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
Re: [PATCH v2] Saving a few cycles, part 3/3
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
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.
>
>> (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.
> 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.
-- Stefan^2.
RE: [PATCH v2] Saving a few cycles, part 3/3
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Fuhrmann [mailto:stefanfuhrmann@alice-dsl.de]
> Sent: zaterdag 1 mei 2010 19:03
> To: dev@subversion.apache.org
> Subject: [PATCH v2] Saving a few cycles, part 3/3
>
> Hi devs,
>
> I reworked the patch set according to the feedback I got
> here on this list. This is what changed in this last part:
>
> * add rationales to the commentary
> * bring svndiff.c changes closer to the original code
> * use svn_ctype_* functions in checksum parser
>
> -- Stefan^2.
>
> [[[
> Various local optimizations. These opportunities became
> visible after significantly optimizing other parts of svn. The
> total savings for a 'svn export' is almost 3 percent on top
> of the previous.
>
> The largest savings can be attributed to the svndiff.c
> changes (~1.5%) and the checksum parser (~1%).
>
> * 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).
In the rest of your patch you can use the enum constants in some places
instead of the magic values to increase some readability. (But I think the
old code didn't do that either).
>
> * subversion/libsvn_delta/compose_delta.c
> (search_offset_index): use size_t to index memory.
> This is mainly a consistency fix but may actually result
> in slightly higher performance due to fewer conversions.
s/size_t/apr_size_t/
And it can also cause a slowdown. But I like to see apr_size_t anyway.
> (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).
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.
>
> * subversion/libsvn_subr/checksum.c
> (svn_checksum_parse_hex): eliminate calls to locale-
> aware CRT functions. At least with MS compilers,
> these are very expensive.
Yes, these call into the OS-libraries on Windows.. Avoiding these helps *a
lot*.
> * subversion/libsvn_subr/stream.c
> (stream_readline): numbytes is invariant until EOF
>
> patch by stefanfuhrmann < at > alice-dsl.de
> ]]]
Bert