You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Martin Hauner <ha...@web.de> on 2004/10/23 13:07:43 UTC
[PATCH]? svn status performance (win32)
Hi,
at work we have a working copy that is not really small. It takes about 25
seconds to run svn status on it (from subcommander, build against the 1.1.x
branch with svn_utf_initialize()).
To find out what takes the time i did run it from Quantify.
Subversion spends a lot of time in io_check_path (-> apr_stat ->
GetFileAttributesEx).
I added a printf(filename) to io_check_path to find out if it is possible to
reduce the number of calls.
For each file in the wc io_check_path is called three times on the file itself
and two times on its property file.
I created three simple patches that reduce the 5 io_check_path calls to a single
call. They reduce the time needed for a status run about ~40% (not quantified,
i am at home). What do you think?
props.patch: removes an unecessary call on the properties file in
svn_wc__load_prop_file. svn_wc__load_prop_file calls svn_wc__load_prop_file
which does the same check.
status.patch: There are two calls to io_check_path here which are used to check
for a 'special' file. As far as I see the only special case is a link. As links
are not used on Win32 i #ifndef'ed them.
statusquestion.patch: Here I removed the io_check_path call in
svn_wc_text_modified_p. I created an svn_wc_text_modified_p2 method which does
no io_check_path call. The original svn_wc_text_modified_p does the check and
then calls svn_wc_text_modified_p2.
All calls to svn_wc_text_modified_p do in some way the io_check_path check. So
maybe just removing the check here is an option?
(The last patch may not cleanly apply, it is hand edited so it is seperated from
the previous patch that modifies the same function)
--
Martin (http://subcommander.tigris.org, a subversion gui client)
Re: [PATCH]? svn status performance (win32)
Posted by Martin Hauner <ha...@web.de>.
Philip Martin wrote:
> Martin Hauner <ha...@web.de> writes:
>>* subversion/libsvn_wc/questions.c
>> (svn_wc_text_modified_p2): copied from svn_wc_text_modified_p and
>> removed the svn_io_check_path check.
>> (svn_wc_text_modified_p): Does the svn_io_check_path check and then
>> calls svn_wc_text_modified_p2.
>>
>>* subversion/libsvn_wc/status.c
>> (assemble_status): call svn_wc_text_modified_p2 instead of
>> svn_wc_text_modified_p.
>
>
> Our naming usually uses foo2 when foo2 is intended to replace foo.
> Since you don't appear to be replacing all the calls that's not really
> the case here, is it?
No.
That patch is just a base for discussion.
I only wanted to modify the status case without impact on the rest of subversion.
I looked through the callers of svn_wc_text_modified_p and as far as
i see all do the check "somewhere". I wasn't sure if it save to simply
remove the check from svn_wc_text_modified_p. So i simply crated a
wrapper.
--
Martin (http://subcommander.tigris.org, a subversion gui client)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]? svn status performance (win32)
Posted by Philip Martin <ph...@codematters.co.uk>.
Martin Hauner <ha...@web.de> writes:
> * subversion/libsvn_wc/props.c
> (svn_wc__load_prop_file): removed an unecessary svn_io_check_path
> call. svn_wc__load_prop_file already does the check.
That looks good; I'll look at committing it.
> Removed svn_io_check_path calls to check for links which are uncessary
> on the Win32 plattform.
>
> * subversion/libsvn_wc/status.c
> (assemble_status): Initializing wc_special and node_special to false.
> Wrapped 'special' checks in #ifndef _WIN32.
>
> Index: subversion/libsvn_wc/status.c
> ===================================================================
> --- subversion/libsvn_wc/status.c (revision 11539)
> +++ subversion/libsvn_wc/status.c (working copy)
> @@ -212,8 +212,8 @@
> svn_boolean_t prop_modified_p = FALSE;
> svn_boolean_t locked_p = FALSE;
> svn_boolean_t switched_p = FALSE;
> - svn_boolean_t wc_special;
> - svn_boolean_t node_special;
> + svn_boolean_t wc_special = FALSE;
> + svn_boolean_t node_special = FALSE;
> svn_node_kind_t kind;
>
> /* Defaults for two main variables. */
> @@ -223,7 +223,6 @@
> /* Check the path kind for PATH. */
> if (path_kind == svn_node_unknown)
> SVN_ERR (svn_io_check_path (path, &path_kind, pool));
> - SVN_ERR (svn_io_check_special_path (path, &kind, &node_special, pool));
>
> if (! entry)
> {
> @@ -305,7 +304,12 @@
> SVN_ERR (svn_wc_props_modified_p (&prop_modified_p, path, adm_access,
> pool));
>
> +#ifndef _WIN32
> + // we don't have links on win32, so we can save some io_check_path calls
> + // on the file itself and its property file.
> + SVN_ERR (svn_io_check_special_path (path, &kind, &node_special, pool));
> SVN_ERR (svn_wc__get_special (&wc_special, path, adm_access, pool));
> +#endif // _WIN32
The rest of the code use WIN32, not _WIN32. I don't do Windows so I
can't really comment, but I don't like the spread of #ifdef code. I
haven't looked in detail, but my initial instinct is that the
conditional code should be inside the functions, not at the call site.
> * subversion/libsvn_wc/questions.c
> (svn_wc_text_modified_p2): copied from svn_wc_text_modified_p and
> removed the svn_io_check_path check.
> (svn_wc_text_modified_p): Does the svn_io_check_path check and then
> calls svn_wc_text_modified_p2.
>
> * subversion/libsvn_wc/status.c
> (assemble_status): call svn_wc_text_modified_p2 instead of
> svn_wc_text_modified_p.
Our naming usually uses foo2 when foo2 is intended to replace foo.
Since you don't appear to be replacing all the calls that's not really
the case here, is it?
The text_modified_p and props_modified_p functions have long been on
my hit list, they are really inefficient in terms of disk io and iconv
stuff (the props_modified_p function is particularly bad). I'd prefer
to see the modified_p functions implemented so that they don't
repeatedly stat(), then all callers would benefit.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]? svn status performance (win32)
Posted by Martin Hauner <ha...@web.de>.
Branko Čibej wrote:
> Well, if I were you I'd repeat the performance measurement now that the
> first patch has been applied. As I said, I measure a 60% (real time)
> speedup with that patch, so it's not exactly obvious that this change
> would make a sgignificant difference.
Ok, i will make some more test runs to see what happens.. I was going
to do it anyway.
--
Martin (http://subcommander.tigris.org, a subversion gui client)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]? svn status performance (win32)
Posted by Branko Čibej <br...@xbc.nu>.
Martin Hauner wrote:
> Branko Čibej wrote:
>
>> This one I don't like on philosophical grounds. If we do make
>> special-file checks platform-specific -- something I don't like,
>> either -- then the ifdef belongs inside svn_io_check_special_path,
>> not in its callers.
>
> I don't really like the #ifdef but it was the easiest for testing.
> A better way would be a has_special_files function which returns false
> for
> Win32 and true for anything else and then add a has_special_files to
> io_check_special to save the apr_stat call.
It shouldn't be a platform-specific #ifdef but a feature-specific one,
as you note. Once that's taken care of, it doesn't matter if it's an if
or an #ifdef. The main problem was adding conditionals to the call site
instead of in the function itself.
>> My guess is that you meant to make the ..._p2 function private in
>> libsvn_wc and have those functions that already do the check and then
>> call svn_wc_text_modified_p call the private function instead.
>
> Yes, much better this way. :) Would it create an acceptable patch?
Well, if I were you I'd repeat the performance measurement now that the
first patch has been applied. As I said, I measure a 60% (real time)
speedup with that patch, so it's not exactly obvious that this change
would make a sgignificant difference.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]? svn status performance (win32)
Posted by Martin Hauner <ha...@web.de>.
Branko Čibej wrote:
> Martin Hauner wrote:
>
>> Hi,
>>
> Actually I suspect this patch gives most of the speedup. I get about 60%
> shorter times on a "svn st -v" of a SVN trunk working copy, compared to
> the 1.1.0 release. It also speeds up my ra_local tests by about 30%,
> which is quite a feat, given that I'm running them on a ramdisk. All in
> all, a good catch.
>
> (Of course, your log message is wrong, but I took care of that. :-)
That's ok with me. ;)
>> status.patch: There are two calls to io_check_path here which are used
>> to check for a 'special' file. As far as I see the only special case
>> is a link. As links are not used on Win32 i #ifndef'ed them.
>
>
> This one I don't like on philosophical grounds. If we do make
> special-file checks platform-specific -- something I don't like, either
> -- then the ifdef belongs inside svn_io_check_special_path, not in its
> callers.
I don't really like the #ifdef but it was the easiest for testing.
A better way would be a has_special_files function which returns false for
Win32 and true for anything else and then add a has_special_files to
io_check_special to save the apr_stat call.
> [..]
> My guess is that you meant to make the ..._p2 function private in
> libsvn_wc and have those functions that already do the check and then
> call svn_wc_text_modified_p call the private function instead.
Yes, much better this way. :) Would it create an acceptable patch?
>> All calls to svn_wc_text_modified_p do in some way the io_check_path
>> check. So maybe just removing the check here is an option?
>
>
> No, because svn_wc_text_modified_p is a public API, and you can't change
> its behaviour.
ah, I see, that makes sense.
--
Martin (http://subcommander.tigris.org, a subversion gui client)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]? svn status performance (win32)
Posted by Branko Čibej <br...@xbc.nu>.
Martin Hauner wrote:
> Hi,
>
> at work we have a working copy that is not really small. It takes
> about 25 seconds to run svn status on it (from subcommander, build
> against the 1.1.x branch with svn_utf_initialize()).
>
> To find out what takes the time i did run it from Quantify.
>
> Subversion spends a lot of time in io_check_path (-> apr_stat ->
> GetFileAttributesEx).
>
> I added a printf(filename) to io_check_path to find out if it is
> possible to reduce the number of calls.
> For each file in the wc io_check_path is called three times on the
> file itself and two times on its property file.
>
> I created three simple patches that reduce the 5 io_check_path calls
> to a single call. They reduce the time needed for a status run about
> ~40% (not quantified,
> i am at home). What do you think?
>
> props.patch: removes an unecessary call on the properties file in
> svn_wc__load_prop_file. svn_wc__load_prop_file calls
> svn_wc__load_prop_file which does the same check.
This patch looks perfectly O.K. to me.
Well, the last SVN_ERR is redundant as you could just return the value
you got from svn_wc__load_prop_file. Committed with this change in
11592. Thanks!
Actually I suspect this patch gives most of the speedup. I get about 60%
shorter times on a "svn st -v" of a SVN trunk working copy, compared to
the 1.1.0 release. It also speeds up my ra_local tests by about 30%,
which is quite a feat, given that I'm running them on a ramdisk. All in
all, a good catch.
(Of course, your log message is wrong, but I took care of that. :-)
> status.patch: There are two calls to io_check_path here which are used
> to check for a 'special' file. As far as I see the only special case
> is a link. As links are not used on Win32 i #ifndef'ed them.
This one I don't like on philosophical grounds. If we do make
special-file checks platform-specific -- something I don't like, either
-- then the ifdef belongs inside svn_io_check_special_path, not in its
callers.
> statusquestion.patch: Here I removed the io_check_path call in
> svn_wc_text_modified_p. I created an svn_wc_text_modified_p2 method
> which does no io_check_path call. The original svn_wc_text_modified_p
> does the check and then calls svn_wc_text_modified_p2.
You didn't actually gain anything by doing this, did you? All you did
was move the check from one function to another. Also creating a new
public API is /not/ a good idea here (not to mention that you didn't
declare it in any header)
My guess is that you meant to make the ..._p2 function private in
libsvn_wc and have those functions that already do the check and then
call svn_wc_text_modified_p call the private function instead.
> All calls to svn_wc_text_modified_p do in some way the io_check_path
> check. So maybe just removing the check here is an option?
No, because svn_wc_text_modified_p is a public API, and you can't change
its behaviour.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org