You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2014/04/13 11:45:03 UTC
svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c
dag.h tree.c
Author: stefan2
Date: Sun Apr 13 09:45:03 2014
New Revision: 1586947
URL: http://svn.apache.org/r1586947
Log:
Improve MT scalability of the FSFS DAG traversal code.
Error objects are a very expensive way to control the control flow
as they carry their own pools, created from a thread-safe root pool.
dag_open should not return an error to open_path if the dirent
cannot be found, pass a NULL node back instead. This eliminates
about 50% of all transitional error objects during log-y operations.
As there are only 2 callers of dag_open, they are easy to adapt.
* subversion/libsvn_fs_fs/dag.h
(svn_fs_fs__dag_clone_root): Document the new error behavior.
* subversion/libsvn_fs_fs/dag.c
(svn_fs_fs__dag_clone_child): This caller actually wants an error,
thus we create it here.
(svn_fs_fs__dag_open): Return NULL if entry cannot be found.
* subversion/libsvn_fs_fs/tree.c
(open_path): Replace error meddling with a check for NULL.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/dag.c
subversion/trunk/subversion/libsvn_fs_fs/dag.h
subversion/trunk/subversion/libsvn_fs_fs/tree.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/dag.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/dag.c?rev=1586947&r1=1586946&r2=1586947&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/dag.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/dag.c Sun Apr 13 09:45:03 2014
@@ -685,6 +685,10 @@ svn_fs_fs__dag_clone_child(dag_node_t **
/* Find the node named NAME in PARENT's entries list if it exists. */
SVN_ERR(svn_fs_fs__dag_open(&cur_entry, parent, name, pool, subpool));
+ if (! cur_entry)
+ return svn_error_createf
+ (SVN_ERR_FS_NOT_FOUND, NULL,
+ "Attempted to open non-existent child node '%s'", name);
/* Check for mutability in the node we found. If it's mutable, we
don't need to clone it. */
@@ -1174,9 +1178,10 @@ svn_fs_fs__dag_open(dag_node_t **child_p
SVN_ERR(dir_entry_id_from_node(&node_id, parent, name,
scratch_pool, scratch_pool));
if (! node_id)
- return svn_error_createf
- (SVN_ERR_FS_NOT_FOUND, NULL,
- "Attempted to open non-existent child node '%s'", name);
+ {
+ *child_p = NULL;
+ return SVN_NO_ERROR;
+ }
/* Make sure that NAME is a single path component. */
if (! svn_path_is_single_path_component(name))
Modified: subversion/trunk/subversion/libsvn_fs_fs/dag.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/dag.h?rev=1586947&r1=1586946&r2=1586947&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/dag.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/dag.h Sun Apr 13 09:45:03 2014
@@ -254,7 +254,8 @@ svn_error_t *svn_fs_fs__dag_clone_root(d
/* Open the node named NAME in the directory PARENT. Set *CHILD_P to
the new node, allocated in RESULT_POOL. NAME must be a single path
- component; it cannot be a slash-separated directory path.
+ component; it cannot be a slash-separated directory path. If NAME does
+ not exist within PARENT, set *CHILD_P to NULL.
*/
svn_error_t *
svn_fs_fs__dag_open(dag_node_t **child_p,
Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1586947&r1=1586946&r2=1586947&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Sun Apr 13 09:45:03 2014
@@ -1033,7 +1033,6 @@ open_path(parent_path_t **parent_path_p,
{
copy_id_inherit_t inherit;
const char *copy_path = NULL;
- svn_error_t *err = SVN_NO_ERROR;
dag_node_t *cached_node = NULL;
/* If we found a directory entry, follow it. First, we
@@ -1047,17 +1046,15 @@ open_path(parent_path_t **parent_path_p,
if (cached_node)
child = cached_node;
else
- err = svn_fs_fs__dag_open(&child, here, entry, pool, iterpool);
+ SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool));
/* "file not found" requires special handling. */
- if (err && err->apr_err == SVN_ERR_FS_NOT_FOUND)
+ if (child == NULL)
{
/* If this was the last path component, and the caller
said it was optional, then don't return an error;
just put a NULL node pointer in the path. */
- svn_error_clear(err);
-
if ((flags & open_path_last_optional)
&& (! next || *next == '\0'))
{
@@ -1073,9 +1070,6 @@ open_path(parent_path_t **parent_path_p,
}
}
- /* Other errors we return normally. */
- SVN_ERR(err);
-
if (flags & open_path_node_only)
{
/* Shortcut: the caller only wan'ts the final DAG node. */
Re: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> Unless there are a lot of string creations to create a vey specific
> error message, I've never seen a construction of svn_error_t * in any
> performance traces...
It is possible: in 2003 I comitted r847626 to avoid an svn_error_t
overhead in the working-copy access-baton code.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
Posted by Julian Foad <ju...@btopenworld.com>.
Ivan Zhakov wrote:
> Julian Foad wrote:
>> It seems the main problem here is simply that this log message summary line gives
>> a false impression about the magnitude of this particular change.
[...]
>> I totally support this particular kind of change. It's simply good interface design.
>
> I completely agree with Julian: the change itself is good, but
> performance should not be justification for it.
I don't want to drag out this thread any longer, but to make sure my position is clear: I think
performance IS a sufficient justification. I also think design style would be a sufficient justification. Either of those alone or both together would be sufficient. (And they are not entirely separate.)
- Julian
> It makes design more
> clear and consistent with other function that have IGNORE_ENOENT
> argument.
Re: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs:
dag.c dag.h tree.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 14 April 2014 12:23, Julian Foad <ju...@btopenworld.com> wrote:
> Bert Huijben wrote:
>>> Author: stefan2
>>> URL: http://svn.apache.org/r1586947
>>> Log:
>>> Improve MT scalability of the FSFS DAG traversal code.
>
> It seems the main problem here is simply that this log message summary line gives a false impression
> about the magnitude of this particular change.
>
>>> Error objects are a very expensive way to control the control flow
>>> as they carry their own pools, created from a thread-safe root pool.
>>>
>>> dag_open should not return an error to open_path if the dirent
>>> cannot be found, pass a NULL node back instead. This eliminates
>>> about 50% of all transitional error objects during log-y operations.
>
> I totally support this particular kind of change. It's simply good interface design.
>
I completely agree with Julian: the change itself is good, but
performance should not be justification for it. It makes design more
clear and consistent with other function that have IGNORE_ENOENT
argument.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
>> Author: stefan2
>> URL: http://svn.apache.org/r1586947
>> Log:
>> Improve MT scalability of the FSFS DAG traversal code.
It seems the main problem here is simply that this log message summary line gives a false impression about the magnitude of this particular change.
>> Error objects are a very expensive way to control the control flow
>> as they carry their own pools, created from a thread-safe root pool.
>>
>> dag_open should not return an error to open_path if the dirent
>> cannot be found, pass a NULL node back instead. This eliminates
>> about 50% of all transitional error objects during log-y operations.
I totally support this particular kind of change. It's simply good interface design.
p.s. A minor commenting nit...
>> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
>> ==========================================================
>> @@ -1047,17 +1046,15 @@ open_path(parent_path_t **parent_path_p,
>> if (cached_node)
>> child = cached_node;
>> else
>> - err = svn_fs_fs__dag_open(&child, here, entry, pool, iterpool);
>> + SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool));
>>
>> /* "file not found" requires special handling. */
That comment there is now redundant ...
>> - if (err && err->apr_err == SVN_ERR_FS_NOT_FOUND)
>> + if (child == NULL)
>> {
>> @@ -1073,9 +1070,6 @@ open_path(parent_path_t **parent_path_p,
>> }
>> }
>>
>> - /* Other errors we return normally. */
... as this one has now gone away.
>> - SVN_ERR(err);
>> -
- Julian
Re: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs:
dag.c dag.h tree.c
Posted by Branko Čibej <br...@wandisco.com>.
On 14.04.2014 17:13, Bert Huijben wrote:
> But are you using 100% the same binaries on the server side and in the client?
>
> (As far as I know most binary providers deliver different binaries towards servers and clients)
>
>
>
> I don’t think we usually compile the client code with memcached support, and a few other server side technologies.
Who's "we" and what do binaries have to do with this project?
Yes, you can compile your stuff with --disable-nls or omit the .mo files
or whatever. Bottom line, this has nothing whatsoever to do with the
source code.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
RE: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
Posted by Bert Huijben <be...@qqmail.nl>.
But are you using 100% the same binaries on the server side and in the client?
(As far as I know most binary providers deliver different binaries towards servers and clients)
I don’t think we usually compile the client code with memcached support, and a few other server side technologies.
If you really want the best support, turning off gettext on the server side (where you usually don’t run your svn and certainly don’t share binaries between them), might make a huge difference.
Bert
From: Branko Čibej [mailto:brane@wandisco.com]
Sent: maandag 14 april 2014 16:51
To: Subversion Development
Subject: RE: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
On 14 Apr 2014 14:55, "Bert Huijben" <bert@qqmail.nl <ma...@qqmail.nl> > wrote:
>
> But why do we use gettext on server processes side in the first place?
FSFS is far from being exclusively server-side. And there are better ways for avoiding gettext than trying to guess whether your code is running in a server process or not. Who's proposing micro-optimisations now?
-- Brane
RE: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs:
dag.c dag.h tree.c
Posted by Branko Čibej <br...@wandisco.com>.
On 14 Apr 2014 14:55, "Bert Huijben" <be...@qqmail.nl> wrote:
>
> But why do we use gettext on server processes side in the first place?
FSFS is far from being exclusively server-side. And there are better ways
for avoiding gettext than trying to guess whether your code is running in a
server process or not. Who's proposing micro-optimisations now?
-- Brane
RE: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
Posted by Bert Huijben <be...@qqmail.nl>.
Can you please improve your log message to tell what you actually did here.
You improved the code flow within some private parts of fsfs to avoid creating errors.
What your log message tells to somebody that is reading the log message for review:
I read the message as
* “HUGE PERFORMANCE IMPROVEMENT FOR MULTITHREADING !!!”
* “Pool creation in error production is expensive and should be avoided”
So you’re telling that you make things less expensive, as the pool creation is so expensive…
But in fact you are not changing any of this, but are avoiding gettext, which doesn’t use apr pools at all. And the pools aren’t in the performance trace.
In the past we tried to use error code only errors for these *expensive* errors. These errors would then only get a (translated) message when their error message was used. That is why all our error codes have default messages.
But why do we use gettext on server processes side in the first place?
We have no way to transfer the locale to the server, so we use the “C” locale anyway. Using gettext in the client makes sense, but on the server not so much.
Bert
From: Stefan Fuhrmann [mailto:stefan.fuhrmann@wandisco.com]
Sent: maandag 14 april 2014 14:01
To: Bert Huijben
Cc: Subversion Development; commits@subversion.apache.org
Subject: Re: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
On Sun, Apr 13, 2014 at 6:18 PM, Bert Huijben <bert@qqmail.nl <ma...@qqmail.nl> > wrote:
> -----Original Message-----
> From: stefan2@apache.org <ma...@apache.org> [mailto:stefan2@apache.org <ma...@apache.org> ]
> Sent: zondag 13 april 2014 11:45
> To: commits@subversion.apache.org <ma...@subversion.apache.org>
> Subject: svn commit: r1586947 - in
> /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
>
> Author: stefan2
> Date: Sun Apr 13 09:45:03 2014
> New Revision: 1586947
>
> URL: http://svn.apache.org/r1586947
> Log:
> Improve MT scalability of the FSFS DAG traversal code.
>
> Error objects are a very expensive way to control the control flow
> as they carry their own pools, created from a thread-safe root pool.
>
> dag_open should not return an error to open_path if the dirent
> cannot be found, pass a NULL node back instead. This eliminates
> about 50% of all transitional error objects during log-y operations.
> As there are only 2 callers of dag_open, they are easy to adapt.
Unless there are a lot of string creations to create a vey specific error message, I've never seen a construction of svn_error_t * in any performance traces...
As the log message said: This code was abusing
error messages for non-erroneous situations (point 1:
really bad design) with 10s of thousands of errors
being created, consumed and re-created (point 2:
inefficient).
The third point is that the implicit dcigettext call as
well as the apr_pool_t creation and destruction for
those error objects require process-wide synchronization.
That limits scalability. I'm currently looking into various
scenarios where a few concurrent requests are already
enough to hog the server or even make it go OOM.
Unless you have some numbers to prove that this helps on more setups than yours, I don't see this as a *single reason* to change current behavior.
If you insist:
Server-side of 'svn log -v -g $tsvn_repo/trunk' (25k revs total in repo)
%Ir #called function
3.72 14062 svn_error_createf.constprop.259
2.72 9370 dcgettext
0.45 14064 svn_error_clear
(plus various small suff)
So, roughly 7% of the instructions are being wasted on
bad interface design. Now imagine what happens to repos
with somewhat more complicated merge histories.
And these numbers do *not* cover the OS sync. overhead.
There could be valid other reasons of course.
Such as the ones mentioned in the log message.
This looks like another case of micro-optimizing certain code which performance critical... I think there is more to gain with other kinds of optimizations, like changing total algorithms to scale better.
Last time I used a different hashing *algorithm*, you still
called it a micro-optimization hoping that a compiler could
do the job that I did in code ...
It wouldn't be the first case where you optimized code that shouldn't have been executed in the first place.
So, did you review that code in the first place?
Note that there is still an open thread on dev@s.a.o <ma...@s.a.o> about reverting a lot of changes like this in fsfs for guaranteed stability. I would say this falls in this same discussion if we decide to revert those.
There has been a thread about 1.9.0alpha1 (multiple threads,
actually). I will start one about FSFS format7 by the end of
this week.
Looking further in your patch makes it appear that this is just a minor refactoring...
Hm. The obvious strategy here would be "read first, post later",
don't you agree?
I know that you work really hard and I fully appreciate
your feedback (and all your code contributions). Just
don't let the stress get the better of you ... ;)
Not sure if the summary really makes sense for that as I don't think it will provide a measurable performance improvement, while the summary makes it appear that it resolves major multithreading problems.
As Julian already pointed out, the initial sentence ("lemma"
in Wiki-speak) may be somewhat of an overstatement. But
I'm in the process of figuring out why some operations e.g.
scale better in Apache than svnserve. This is one of the
things that came up. Now, both are faster with svnserve
seeing a slightly larger improvement.
-- Stefan^2.
RE: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
Posted by Bert Huijben <be...@qqmail.nl>.
Can you please improve your log message to tell what you actually did here.
You improved the code flow within some private parts of fsfs to avoid creating errors.
What your log message tells to somebody that is reading the log message for review:
I read the message as
* “HUGE PERFORMANCE IMPROVEMENT FOR MULTITHREADING !!!”
* “Pool creation in error production is expensive and should be avoided”
So you’re telling that you make things less expensive, as the pool creation is so expensive…
But in fact you are not changing any of this, but are avoiding gettext, which doesn’t use apr pools at all. And the pools aren’t in the performance trace.
In the past we tried to use error code only errors for these *expensive* errors. These errors would then only get a (translated) message when their error message was used. That is why all our error codes have default messages.
But why do we use gettext on server processes side in the first place?
We have no way to transfer the locale to the server, so we use the “C” locale anyway. Using gettext in the client makes sense, but on the server not so much.
Bert
From: Stefan Fuhrmann [mailto:stefan.fuhrmann@wandisco.com]
Sent: maandag 14 april 2014 14:01
To: Bert Huijben
Cc: Subversion Development; commits@subversion.apache.org
Subject: Re: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
On Sun, Apr 13, 2014 at 6:18 PM, Bert Huijben <bert@qqmail.nl <ma...@qqmail.nl> > wrote:
> -----Original Message-----
> From: stefan2@apache.org <ma...@apache.org> [mailto:stefan2@apache.org <ma...@apache.org> ]
> Sent: zondag 13 april 2014 11:45
> To: commits@subversion.apache.org <ma...@subversion.apache.org>
> Subject: svn commit: r1586947 - in
> /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
>
> Author: stefan2
> Date: Sun Apr 13 09:45:03 2014
> New Revision: 1586947
>
> URL: http://svn.apache.org/r1586947
> Log:
> Improve MT scalability of the FSFS DAG traversal code.
>
> Error objects are a very expensive way to control the control flow
> as they carry their own pools, created from a thread-safe root pool.
>
> dag_open should not return an error to open_path if the dirent
> cannot be found, pass a NULL node back instead. This eliminates
> about 50% of all transitional error objects during log-y operations.
> As there are only 2 callers of dag_open, they are easy to adapt.
Unless there are a lot of string creations to create a vey specific error message, I've never seen a construction of svn_error_t * in any performance traces...
As the log message said: This code was abusing
error messages for non-erroneous situations (point 1:
really bad design) with 10s of thousands of errors
being created, consumed and re-created (point 2:
inefficient).
The third point is that the implicit dcigettext call as
well as the apr_pool_t creation and destruction for
those error objects require process-wide synchronization.
That limits scalability. I'm currently looking into various
scenarios where a few concurrent requests are already
enough to hog the server or even make it go OOM.
Unless you have some numbers to prove that this helps on more setups than yours, I don't see this as a *single reason* to change current behavior.
If you insist:
Server-side of 'svn log -v -g $tsvn_repo/trunk' (25k revs total in repo)
%Ir #called function
3.72 14062 svn_error_createf.constprop.259
2.72 9370 dcgettext
0.45 14064 svn_error_clear
(plus various small suff)
So, roughly 7% of the instructions are being wasted on
bad interface design. Now imagine what happens to repos
with somewhat more complicated merge histories.
And these numbers do *not* cover the OS sync. overhead.
There could be valid other reasons of course.
Such as the ones mentioned in the log message.
This looks like another case of micro-optimizing certain code which performance critical... I think there is more to gain with other kinds of optimizations, like changing total algorithms to scale better.
Last time I used a different hashing *algorithm*, you still
called it a micro-optimization hoping that a compiler could
do the job that I did in code ...
It wouldn't be the first case where you optimized code that shouldn't have been executed in the first place.
So, did you review that code in the first place?
Note that there is still an open thread on dev@s.a.o <ma...@s.a.o> about reverting a lot of changes like this in fsfs for guaranteed stability. I would say this falls in this same discussion if we decide to revert those.
There has been a thread about 1.9.0alpha1 (multiple threads,
actually). I will start one about FSFS format7 by the end of
this week.
Looking further in your patch makes it appear that this is just a minor refactoring...
Hm. The obvious strategy here would be "read first, post later",
don't you agree?
I know that you work really hard and I fully appreciate
your feedback (and all your code contributions). Just
don't let the stress get the better of you ... ;)
Not sure if the summary really makes sense for that as I don't think it will provide a measurable performance improvement, while the summary makes it appear that it resolves major multithreading problems.
As Julian already pointed out, the initial sentence ("lemma"
in Wiki-speak) may be somewhat of an overstatement. But
I'm in the process of figuring out why some operations e.g.
scale better in Apache than svnserve. This is one of the
things that came up. Now, both are faster with svnserve
seeing a slightly larger improvement.
-- Stefan^2.
Re: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs:
dag.c dag.h tree.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Apr 13, 2014 at 6:18 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
> > -----Original Message-----
> > From: stefan2@apache.org [mailto:stefan2@apache.org]
> > Sent: zondag 13 april 2014 11:45
> > To: commits@subversion.apache.org
> > Subject: svn commit: r1586947 - in
> > /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
> >
> > Author: stefan2
> > Date: Sun Apr 13 09:45:03 2014
> > New Revision: 1586947
> >
> > URL: http://svn.apache.org/r1586947
> > Log:
> > Improve MT scalability of the FSFS DAG traversal code.
> >
> > Error objects are a very expensive way to control the control flow
> > as they carry their own pools, created from a thread-safe root pool.
> >
> > dag_open should not return an error to open_path if the dirent
> > cannot be found, pass a NULL node back instead. This eliminates
> > about 50% of all transitional error objects during log-y operations.
> > As there are only 2 callers of dag_open, they are easy to adapt.
>
> Unless there are a lot of string creations to create a vey specific error
> message, I've never seen a construction of svn_error_t * in any performance
> traces...
>
As the log message said: This code was abusing
error messages for non-erroneous situations (point 1:
really bad design) with 10s of thousands of errors
being created, consumed and re-created (point 2:
inefficient).
The third point is that the implicit dcigettext call as
well as the apr_pool_t creation and destruction for
those error objects require process-wide synchronization.
That limits scalability. I'm currently looking into various
scenarios where a few concurrent requests are already
enough to hog the server or even make it go OOM.
>
> Unless you have some numbers to prove that this helps on more setups than
> yours, I don't see this as a *single reason* to change current behavior.
>
If you insist:
Server-side of 'svn log -v -g $tsvn_repo/trunk' (25k revs total in repo)
%Ir #called function
3.72 14062 svn_error_createf.constprop.259
2.72 9370 dcgettext
0.45 14064 svn_error_clear
(plus various small suff)
So, roughly 7% of the instructions are being wasted on
bad interface design. Now imagine what happens to repos
with somewhat more complicated merge histories.
And these numbers do *not* cover the OS sync. overhead.
>
> There could be valid other reasons of course.
>
Such as the ones mentioned in the log message.
> This looks like another case of micro-optimizing certain code which
> performance critical... I think there is more to gain with other kinds of
> optimizations, like changing total algorithms to scale better.
Last time I used a different hashing *algorithm*, you still
called it a micro-optimization hoping that a compiler could
do the job that I did in code ...
> It wouldn't be the first case where you optimized code that shouldn't have
> been executed in the first place.
>
So, did you review that code in the first place?
Note that there is still an open thread on dev@s.a.o about reverting a lot
> of changes like this in fsfs for guaranteed stability. I would say this
> falls in this same discussion if we decide to revert those.
>
There has been a thread about 1.9.0alpha1 (multiple threads,
actually). I will start one about FSFS format7 by the end of
this week.
> Looking further in your patch makes it appear that this is just a minor
> refactoring...
Hm. The obvious strategy here would be "read first, post later",
don't you agree?
I know that you work really hard and I fully appreciate
your feedback (and all your code contributions). Just
don't let the stress get the better of you ... ;)
> Not sure if the summary really makes sense for that as I don't think it
> will provide a measurable performance improvement, while the summary makes
> it appear that it resolves major multithreading problems.
>
As Julian already pointed out, the initial sentence ("lemma"
in Wiki-speak) may be somewhat of an overstatement. But
I'm in the process of figuring out why some operations e.g.
scale better in Apache than svnserve. This is one of the
things that came up. Now, both are faster with svnserve
seeing a slightly larger improvement.
-- Stefan^2.
Re: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs:
dag.c dag.h tree.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Apr 13, 2014 at 6:18 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
> > -----Original Message-----
> > From: stefan2@apache.org [mailto:stefan2@apache.org]
> > Sent: zondag 13 april 2014 11:45
> > To: commits@subversion.apache.org
> > Subject: svn commit: r1586947 - in
> > /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
> >
> > Author: stefan2
> > Date: Sun Apr 13 09:45:03 2014
> > New Revision: 1586947
> >
> > URL: http://svn.apache.org/r1586947
> > Log:
> > Improve MT scalability of the FSFS DAG traversal code.
> >
> > Error objects are a very expensive way to control the control flow
> > as they carry their own pools, created from a thread-safe root pool.
> >
> > dag_open should not return an error to open_path if the dirent
> > cannot be found, pass a NULL node back instead. This eliminates
> > about 50% of all transitional error objects during log-y operations.
> > As there are only 2 callers of dag_open, they are easy to adapt.
>
> Unless there are a lot of string creations to create a vey specific error
> message, I've never seen a construction of svn_error_t * in any performance
> traces...
>
As the log message said: This code was abusing
error messages for non-erroneous situations (point 1:
really bad design) with 10s of thousands of errors
being created, consumed and re-created (point 2:
inefficient).
The third point is that the implicit dcigettext call as
well as the apr_pool_t creation and destruction for
those error objects require process-wide synchronization.
That limits scalability. I'm currently looking into various
scenarios where a few concurrent requests are already
enough to hog the server or even make it go OOM.
>
> Unless you have some numbers to prove that this helps on more setups than
> yours, I don't see this as a *single reason* to change current behavior.
>
If you insist:
Server-side of 'svn log -v -g $tsvn_repo/trunk' (25k revs total in repo)
%Ir #called function
3.72 14062 svn_error_createf.constprop.259
2.72 9370 dcgettext
0.45 14064 svn_error_clear
(plus various small suff)
So, roughly 7% of the instructions are being wasted on
bad interface design. Now imagine what happens to repos
with somewhat more complicated merge histories.
And these numbers do *not* cover the OS sync. overhead.
>
> There could be valid other reasons of course.
>
Such as the ones mentioned in the log message.
> This looks like another case of micro-optimizing certain code which
> performance critical... I think there is more to gain with other kinds of
> optimizations, like changing total algorithms to scale better.
Last time I used a different hashing *algorithm*, you still
called it a micro-optimization hoping that a compiler could
do the job that I did in code ...
> It wouldn't be the first case where you optimized code that shouldn't have
> been executed in the first place.
>
So, did you review that code in the first place?
Note that there is still an open thread on dev@s.a.o about reverting a lot
> of changes like this in fsfs for guaranteed stability. I would say this
> falls in this same discussion if we decide to revert those.
>
There has been a thread about 1.9.0alpha1 (multiple threads,
actually). I will start one about FSFS format7 by the end of
this week.
> Looking further in your patch makes it appear that this is just a minor
> refactoring...
Hm. The obvious strategy here would be "read first, post later",
don't you agree?
I know that you work really hard and I fully appreciate
your feedback (and all your code contributions). Just
don't let the stress get the better of you ... ;)
> Not sure if the summary really makes sense for that as I don't think it
> will provide a measurable performance improvement, while the summary makes
> it appear that it resolves major multithreading problems.
>
As Julian already pointed out, the initial sentence ("lemma"
in Wiki-speak) may be somewhat of an overstatement. But
I'm in the process of figuring out why some operations e.g.
scale better in Apache than svnserve. This is one of the
things that came up. Now, both are faster with svnserve
seeing a slightly larger improvement.
-- Stefan^2.
RE: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: stefan2@apache.org [mailto:stefan2@apache.org]
> Sent: zondag 13 april 2014 11:45
> To: commits@subversion.apache.org
> Subject: svn commit: r1586947 - in
> /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
>
> Author: stefan2
> Date: Sun Apr 13 09:45:03 2014
> New Revision: 1586947
>
> URL: http://svn.apache.org/r1586947
> Log:
> Improve MT scalability of the FSFS DAG traversal code.
>
> Error objects are a very expensive way to control the control flow
> as they carry their own pools, created from a thread-safe root pool.
>
> dag_open should not return an error to open_path if the dirent
> cannot be found, pass a NULL node back instead. This eliminates
> about 50% of all transitional error objects during log-y operations.
> As there are only 2 callers of dag_open, they are easy to adapt.
Unless there are a lot of string creations to create a vey specific error message, I've never seen a construction of svn_error_t * in any performance traces...
Unless you have some numbers to prove that this helps on more setups than yours, I don't see this as a *single reason* to change current behavior.
There could be valid other reasons of course.
This looks like another case of micro-optimizing certain code which performance critical... I think there is more to gain with other kinds of optimizations, like changing total algorithms to scale better. It wouldn't be the first case where you optimized code that shouldn't have been executed in the first place.
Note that there is still an open thread on dev@s.a.o about reverting a lot of changes like this in fsfs for guaranteed stability. I would say this falls in this same discussion if we decide to revert those.
Looking further in your patch makes it appear that this is just a minor refactoring... Not sure if the summary really makes sense for that as I don't think it will provide a measurable performance improvement, while the summary makes it appear that it resolves major multithreading problems.
Bert
>
> * subversion/libsvn_fs_fs/dag.h
> (svn_fs_fs__dag_clone_root): Document the new error behavior.
>
> * subversion/libsvn_fs_fs/dag.c
> (svn_fs_fs__dag_clone_child): This caller actually wants an error,
> thus we create it here.
> (svn_fs_fs__dag_open): Return NULL if entry cannot be found.
>
> * subversion/libsvn_fs_fs/tree.c
> (open_path): Replace error meddling with a check for NULL.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/dag.c
> subversion/trunk/subversion/libsvn_fs_fs/dag.h
> subversion/trunk/subversion/libsvn_fs_fs/tree.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/dag.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/da
> g.c?rev=1586947&r1=1586946&r2=1586947&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/dag.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/dag.c Sun Apr 13 09:45:03
> 2014
> @@ -685,6 +685,10 @@ svn_fs_fs__dag_clone_child(dag_node_t **
>
> /* Find the node named NAME in PARENT's entries list if it exists. */
> SVN_ERR(svn_fs_fs__dag_open(&cur_entry, parent, name, pool,
> subpool));
> + if (! cur_entry)
> + return svn_error_createf
> + (SVN_ERR_FS_NOT_FOUND, NULL,
> + "Attempted to open non-existent child node '%s'", name);
>
> /* Check for mutability in the node we found. If it's mutable, we
> don't need to clone it. */
> @@ -1174,9 +1178,10 @@ svn_fs_fs__dag_open(dag_node_t **child_p
> SVN_ERR(dir_entry_id_from_node(&node_id, parent, name,
> scratch_pool, scratch_pool));
> if (! node_id)
> - return svn_error_createf
> - (SVN_ERR_FS_NOT_FOUND, NULL,
> - "Attempted to open non-existent child node '%s'", name);
> + {
> + *child_p = NULL;
> + return SVN_NO_ERROR;
> + }
>
> /* Make sure that NAME is a single path component. */
> if (! svn_path_is_single_path_component(name))
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/dag.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/da
> g.h?rev=1586947&r1=1586946&r2=1586947&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/dag.h (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/dag.h Sun Apr 13 09:45:03
> 2014
> @@ -254,7 +254,8 @@ svn_error_t *svn_fs_fs__dag_clone_root(d
>
> /* Open the node named NAME in the directory PARENT. Set *CHILD_P to
> the new node, allocated in RESULT_POOL. NAME must be a single path
> - component; it cannot be a slash-separated directory path.
> + component; it cannot be a slash-separated directory path. If NAME does
> + not exist within PARENT, set *CHILD_P to NULL.
> */
> svn_error_t *
> svn_fs_fs__dag_open(dag_node_t **child_p,
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tr
> ee.c?rev=1586947&r1=1586946&r2=1586947&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Sun Apr 13 09:45:03
> 2014
> @@ -1033,7 +1033,6 @@ open_path(parent_path_t **parent_path_p,
> {
> copy_id_inherit_t inherit;
> const char *copy_path = NULL;
> - svn_error_t *err = SVN_NO_ERROR;
> dag_node_t *cached_node = NULL;
>
> /* If we found a directory entry, follow it. First, we
> @@ -1047,17 +1046,15 @@ open_path(parent_path_t **parent_path_p,
> if (cached_node)
> child = cached_node;
> else
> - err = svn_fs_fs__dag_open(&child, here, entry, pool, iterpool);
> + SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool));
>
> /* "file not found" requires special handling. */
> - if (err && err->apr_err == SVN_ERR_FS_NOT_FOUND)
> + if (child == NULL)
> {
> /* If this was the last path component, and the caller
> said it was optional, then don't return an error;
> just put a NULL node pointer in the path. */
>
> - svn_error_clear(err);
> -
> if ((flags & open_path_last_optional)
> && (! next || *next == '\0'))
> {
> @@ -1073,9 +1070,6 @@ open_path(parent_path_t **parent_path_p,
> }
> }
>
> - /* Other errors we return normally. */
> - SVN_ERR(err);
> -
> if (flags & open_path_node_only)
> {
> /* Shortcut: the caller only wan'ts the final DAG node. */
>
RE: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: stefan2@apache.org [mailto:stefan2@apache.org]
> Sent: zondag 13 april 2014 11:45
> To: commits@subversion.apache.org
> Subject: svn commit: r1586947 - in
> /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
>
> Author: stefan2
> Date: Sun Apr 13 09:45:03 2014
> New Revision: 1586947
>
> URL: http://svn.apache.org/r1586947
> Log:
> Improve MT scalability of the FSFS DAG traversal code.
>
> Error objects are a very expensive way to control the control flow
> as they carry their own pools, created from a thread-safe root pool.
>
> dag_open should not return an error to open_path if the dirent
> cannot be found, pass a NULL node back instead. This eliminates
> about 50% of all transitional error objects during log-y operations.
> As there are only 2 callers of dag_open, they are easy to adapt.
Unless there are a lot of string creations to create a vey specific error message, I've never seen a construction of svn_error_t * in any performance traces...
Unless you have some numbers to prove that this helps on more setups than yours, I don't see this as a *single reason* to change current behavior.
There could be valid other reasons of course.
This looks like another case of micro-optimizing certain code which performance critical... I think there is more to gain with other kinds of optimizations, like changing total algorithms to scale better. It wouldn't be the first case where you optimized code that shouldn't have been executed in the first place.
Note that there is still an open thread on dev@s.a.o about reverting a lot of changes like this in fsfs for guaranteed stability. I would say this falls in this same discussion if we decide to revert those.
Looking further in your patch makes it appear that this is just a minor refactoring... Not sure if the summary really makes sense for that as I don't think it will provide a measurable performance improvement, while the summary makes it appear that it resolves major multithreading problems.
Bert
>
> * subversion/libsvn_fs_fs/dag.h
> (svn_fs_fs__dag_clone_root): Document the new error behavior.
>
> * subversion/libsvn_fs_fs/dag.c
> (svn_fs_fs__dag_clone_child): This caller actually wants an error,
> thus we create it here.
> (svn_fs_fs__dag_open): Return NULL if entry cannot be found.
>
> * subversion/libsvn_fs_fs/tree.c
> (open_path): Replace error meddling with a check for NULL.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/dag.c
> subversion/trunk/subversion/libsvn_fs_fs/dag.h
> subversion/trunk/subversion/libsvn_fs_fs/tree.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/dag.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/da
> g.c?rev=1586947&r1=1586946&r2=1586947&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/dag.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/dag.c Sun Apr 13 09:45:03
> 2014
> @@ -685,6 +685,10 @@ svn_fs_fs__dag_clone_child(dag_node_t **
>
> /* Find the node named NAME in PARENT's entries list if it exists. */
> SVN_ERR(svn_fs_fs__dag_open(&cur_entry, parent, name, pool,
> subpool));
> + if (! cur_entry)
> + return svn_error_createf
> + (SVN_ERR_FS_NOT_FOUND, NULL,
> + "Attempted to open non-existent child node '%s'", name);
>
> /* Check for mutability in the node we found. If it's mutable, we
> don't need to clone it. */
> @@ -1174,9 +1178,10 @@ svn_fs_fs__dag_open(dag_node_t **child_p
> SVN_ERR(dir_entry_id_from_node(&node_id, parent, name,
> scratch_pool, scratch_pool));
> if (! node_id)
> - return svn_error_createf
> - (SVN_ERR_FS_NOT_FOUND, NULL,
> - "Attempted to open non-existent child node '%s'", name);
> + {
> + *child_p = NULL;
> + return SVN_NO_ERROR;
> + }
>
> /* Make sure that NAME is a single path component. */
> if (! svn_path_is_single_path_component(name))
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/dag.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/da
> g.h?rev=1586947&r1=1586946&r2=1586947&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/dag.h (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/dag.h Sun Apr 13 09:45:03
> 2014
> @@ -254,7 +254,8 @@ svn_error_t *svn_fs_fs__dag_clone_root(d
>
> /* Open the node named NAME in the directory PARENT. Set *CHILD_P to
> the new node, allocated in RESULT_POOL. NAME must be a single path
> - component; it cannot be a slash-separated directory path.
> + component; it cannot be a slash-separated directory path. If NAME does
> + not exist within PARENT, set *CHILD_P to NULL.
> */
> svn_error_t *
> svn_fs_fs__dag_open(dag_node_t **child_p,
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tr
> ee.c?rev=1586947&r1=1586946&r2=1586947&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Sun Apr 13 09:45:03
> 2014
> @@ -1033,7 +1033,6 @@ open_path(parent_path_t **parent_path_p,
> {
> copy_id_inherit_t inherit;
> const char *copy_path = NULL;
> - svn_error_t *err = SVN_NO_ERROR;
> dag_node_t *cached_node = NULL;
>
> /* If we found a directory entry, follow it. First, we
> @@ -1047,17 +1046,15 @@ open_path(parent_path_t **parent_path_p,
> if (cached_node)
> child = cached_node;
> else
> - err = svn_fs_fs__dag_open(&child, here, entry, pool, iterpool);
> + SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool));
>
> /* "file not found" requires special handling. */
> - if (err && err->apr_err == SVN_ERR_FS_NOT_FOUND)
> + if (child == NULL)
> {
> /* If this was the last path component, and the caller
> said it was optional, then don't return an error;
> just put a NULL node pointer in the path. */
>
> - svn_error_clear(err);
> -
> if ((flags & open_path_last_optional)
> && (! next || *next == '\0'))
> {
> @@ -1073,9 +1070,6 @@ open_path(parent_path_t **parent_path_p,
> }
> }
>
> - /* Other errors we return normally. */
> - SVN_ERR(err);
> -
> if (flags & open_path_node_only)
> {
> /* Shortcut: the caller only wan'ts the final DAG node. */
>