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 2012/05/31 09:41:38 UTC
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
On Wed, May 30, 2012 at 8:52 PM, <rh...@apache.org> wrote:
> Author: rhuijben
> Date: Wed May 30 16:52:36 2012
> New Revision: 1344347
>
> URL: http://svn.apache.org/viewvc?rev=1344347&view=rev
> Log:
> Remove about 15% of the cpu overhead of svnserve and httpd while running the
> test suite by introducing a very simple parser buffer in the config file parser
> code.
>
> The nice clean stream translation code had an 7 times overhead on this very
> performance critical code path, while clients of svnserve and httpd were
> waiting. (Before this patch the this code path used 22% of the cpu time,
> now just 3%)
>
Great improvement, but it may be worth to factor out this code to
special stream like svn_stream_buffered().
--
Ivan Zhakov
RE: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com]
> Sent: donderdag 31 mei 2012 9:49
> To: Ivan Zhakov
> Cc: dev@subversion.apache.org; Bert Huijben
> Subject: Re: svn commit: r1344347 -
> /subversion/trunk/subversion/libsvn_subr/config_file.c
>
> On Thu, May 31, 2012 at 3:41 AM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> > On Wed, May 30, 2012 at 8:52 PM, <rh...@apache.org> wrote:
> >> Author: rhuijben
> >> Date: Wed May 30 16:52:36 2012
> >> New Revision: 1344347
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1344347&view=rev
> >> Log:
> >> Remove about 15% of the cpu overhead of svnserve and httpd while
> running the
> >> test suite by introducing a very simple parser buffer in the config
file
> parser
> >> code.
> >>
> >> The nice clean stream translation code had an 7 times overhead on this
> very
> >> performance critical code path, while clients of svnserve and httpd
were
> >> waiting. (Before this patch the this code path used 22% of the cpu
time,
> >> now just 3%)
> >
> > Great improvement, but it may be worth to factor out this code to
> > special stream like svn_stream_buffered().
>
> When I saw this change, my first thought was, "wtf? just read the
> whole damned file into memory". (I just haven't had time to look into
> this to provide some actual advice beyond amazement)
>
> Config files are NOT 100 megabytes. Read the damned things into memory
> in one giant piece. I/O is reduced, memory is available. Everybody
> wins.
I'm sure it could use further improvements. I don't know what really large
authz files some users have, but reading in 10 Kbyte chunks as it does now
gives a huge boost by itself.
The current code assumes the line endings have at least a '\n' (and ignores
'\r'), which would work for any typical user, but we used to support '\r' by
itself, because that is what our translated streams do.
I'm not sure if we should call this a regression as we probably never really
intended to support that. (And the Mac switched to '\n' years ago)
Most of the server side time was spend in reading the FSFS config files. I
didn't know we had that many user editable files in a typical repository and
all those commented lines explaining the flags were (and are) the
performance killer.
Bert
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> On Thu, May 31, 2012 at 4:39 AM, Philip Martin
> <ph...@wandisco.com> wrote:
>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>
>>> Config files are also used for authz settings and they can be even
>>> more than 100 MB in real world scenarios.
>>
>> Yes, particularly for setups that use SVNParentPath but not the new
>> AuthzSVNReposRelativeAccessFile.
>
> Woah. Wait a second here.
>
> My understanding is that these are read on *every* request (per
> Daniel). Are you suggesting that we are reading/parsing 100 Mb on
> every request?
>
> There is something wrong here. 100 Mb config files are flat out wrong.
> We should be doing better. And if we are *actually* parsing those
> per-request, then we've gone off the deep-end.
We cache the parsed data in the connection pool, see
mod_authz_svn.c:get_access_conf, so it's per-connection rather than
per-request. That's still not brilliant.
I'm not sure whether the SVNParentPath case discards parsed data for
repositories that are not the one in the current request.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Fri, Jun 1, 2012 at 9:58 AM, Philip Martin
<ph...@wandisco.com> wrote:
> SVNParentPath and AuthzSVNReposRelativeAccessFile make doing it at
> config-time a bit complicated. Would the server scan the SVNParentPath
> directory to locate all the repositories? A repository created later
> would require a reload?
For those two directives, it'd just need to walk the children
directories...so, that's feasible to do at config-time. And, yes,
without a validity check at connection/request-time, we'd need to do a
graceful whenever the repositories or authz files change.
Worst case scenario is that we could load it up at config-time, if it
is invalid, we just fallback to doing it on a per-connection basis.
I'm not entirely sure whether we'd be able to atomically update the
shared config structure if we wanted to stash it in the config object
- though, I guess we could do that too, but it'd add a bit of
complexity that may or may not be worth it.
I don't know how much of a real-world penalty this parsing actually
has (Bert seems to think it is quite significant), but if we wanted to
minimize the I/O, that's the path I think we should take. -- justin
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
Posted by Philip Martin <ph...@wandisco.com>.
Justin Erenkrantz <ju...@erenkrantz.com> writes:
> On Thu, May 31, 2012 at 2:47 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> Currently Subversion reads authz file per-connection, not per-request.
>
> Even that's probably still wrong. A better solution would be to parse
> it at config-time - but, that would mean we'd want to do some level of
> checking/invalidity on a per-connection basis...or, I would probably
> just add a directive which tells the module to skip the
> per-connection/request check and require an httpd config reload (which
> can be done gracefully) to re-load the SVN authz file.
SVNParentPath and AuthzSVNReposRelativeAccessFile make doing it at
config-time a bit complicated. Would the server scan the SVNParentPath
directory to locate all the repositories? A repository created later
would require a reload?
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Thu, May 31, 2012 at 2:47 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> Currently Subversion reads authz file per-connection, not per-request.
Even that's probably still wrong. A better solution would be to parse
it at config-time - but, that would mean we'd want to do some level of
checking/invalidity on a per-connection basis...or, I would probably
just add a directive which tells the module to skip the
per-connection/request check and require an httpd config reload (which
can be done gracefully) to re-load the SVN authz file.
My $.02. -- justin
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, May 31, 2012 at 1:27 PM, Greg Stein <gs...@gmail.com> wrote:
> On Thu, May 31, 2012 at 4:39 AM, Philip Martin
> <ph...@wandisco.com> wrote:
>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>
>>> Config files are also used for authz settings and they can be even
>>> more than 100 MB in real world scenarios.
>>
>> Yes, particularly for setups that use SVNParentPath but not the new
>> AuthzSVNReposRelativeAccessFile.
>
> Woah. Wait a second here.
>
> My understanding is that these are read on *every* request (per
> Daniel). Are you suggesting that we are reading/parsing 100 Mb on
> every request?
>
> There is something wrong here. 100 Mb config files are flat out wrong.
> We should be doing better. And if we are *actually* parsing those
> per-request, then we've gone off the deep-end.
>
> ???
Currently Subversion reads authz file per-connection, not per-request.
--
Ivan Zhakov
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
Posted by Greg Stein <gs...@gmail.com>.
On Thu, May 31, 2012 at 4:39 AM, Philip Martin
<ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> Config files are also used for authz settings and they can be even
>> more than 100 MB in real world scenarios.
>
> Yes, particularly for setups that use SVNParentPath but not the new
> AuthzSVNReposRelativeAccessFile.
Woah. Wait a second here.
My understanding is that these are read on *every* request (per
Daniel). Are you suggesting that we are reading/parsing 100 Mb on
every request?
There is something wrong here. 100 Mb config files are flat out wrong.
We should be doing better. And if we are *actually* parsing those
per-request, then we've gone off the deep-end.
???
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> Config files are also used for authz settings and they can be even
> more than 100 MB in real world scenarios.
Yes, particularly for setups that use SVNParentPath but not the new
AuthzSVNReposRelativeAccessFile.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, May 31, 2012 at 11:49 AM, Greg Stein <gs...@gmail.com> wrote:
> On Thu, May 31, 2012 at 3:41 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On Wed, May 30, 2012 at 8:52 PM, <rh...@apache.org> wrote:
>>> Author: rhuijben
>>> Date: Wed May 30 16:52:36 2012
>>> New Revision: 1344347
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1344347&view=rev
>>> Log:
>>> Remove about 15% of the cpu overhead of svnserve and httpd while running the
>>> test suite by introducing a very simple parser buffer in the config file parser
>>> code.
>>>
>>> The nice clean stream translation code had an 7 times overhead on this very
>>> performance critical code path, while clients of svnserve and httpd were
>>> waiting. (Before this patch the this code path used 22% of the cpu time,
>>> now just 3%)
>>
>> Great improvement, but it may be worth to factor out this code to
>> special stream like svn_stream_buffered().
>
> When I saw this change, my first thought was, "wtf? just read the
> whole damned file into memory". (I just haven't had time to look into
> this to provide some actual advice beyond amazement)
>
> Config files are NOT 100 megabytes. Read the damned things into memory
> in one giant piece. I/O is reduced, memory is available. Everybody
> wins.
>
Config files are also used for authz settings and they can be even
more than 100 MB in real world scenarios.
--
Ivan Zhakov
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
Posted by Greg Stein <gs...@gmail.com>.
On Thu, May 31, 2012 at 3:41 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Wed, May 30, 2012 at 8:52 PM, <rh...@apache.org> wrote:
>> Author: rhuijben
>> Date: Wed May 30 16:52:36 2012
>> New Revision: 1344347
>>
>> URL: http://svn.apache.org/viewvc?rev=1344347&view=rev
>> Log:
>> Remove about 15% of the cpu overhead of svnserve and httpd while running the
>> test suite by introducing a very simple parser buffer in the config file parser
>> code.
>>
>> The nice clean stream translation code had an 7 times overhead on this very
>> performance critical code path, while clients of svnserve and httpd were
>> waiting. (Before this patch the this code path used 22% of the cpu time,
>> now just 3%)
>
> Great improvement, but it may be worth to factor out this code to
> special stream like svn_stream_buffered().
When I saw this change, my first thought was, "wtf? just read the
whole damned file into memory". (I just haven't had time to look into
this to provide some actual advice beyond amazement)
Config files are NOT 100 megabytes. Read the damned things into memory
in one giant piece. I/O is reduced, memory is available. Everybody
wins.
-g