RE: svn commit: r1586947 - in /subversion/trunk/subversion/libsvn_fs_fs: dag.c dag.h tree.c
From: Bert Huijben <bert_at_qqmail.nl>
Date: Mon, 14 Apr 2014 14:54:44 +0200
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.
From: Stefan Fuhrmann [mailto:stefan.fuhrmann_at_wandisco.com]
On Sun, Apr 13, 2014 at 6:18 PM, Bert Huijben <bert_at_qqmail.nl <mailto:bert_at_qqmail.nl> > wrote:
> -----Original Message-----
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
The third point is that the implicit dcigettext call as
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
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
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_at_s.a.o <mailto:dev_at_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,
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",
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"
I'm in the process of figuring out why some operations e.g.
seeing a slightly larger improvement.
This is an archived mail posted to the Subversion Dev mailing list.