Re: [PATCH] issue #3719 fix slow large checkouts on Windows

[Neil Bird <ne...@jibbyjobby.co.uk>]

Around about 08/02/11 10:01, Neil Bird typed ...
> Around about 08/02/11 09:03, Daniel Shahaf typed ...
>> Thanks. However, to clarify, I'm not specifically interested in the
>> "give us N revisions" form; I'm just interested in seeing a coherent
>> patch at the end, and wanted to ensure you haven't forgotten that in
>> depth of coding.
>
> It's just going to take longer that I planned :)

   OK, I tried back-porting (svn merge) actual trunk revisions to get a 
working fix in place, but I stopped when I hit 7 of them (just for this one 
change) and knew (by the volume of conflicts I was having to patch) that it 
was a no-goer.

   Even if I had got it compiling and working, there would have been so many 
tertiary changes that it would have been difficult to prove that it wasn't 
breaking more than it fixed (there are an awful lot of secondary & important 
fixes in trunk relating to the initial re-working;  I'd have almost 
certainly missed one).

   All that added to the fact that the revision numbers seem to have all 
changed (the move from Tigris to Apache?), so even when the comments *do* 
say that they're fixing blah from rev. xxx, the xxx is wrong.


   So, I'm going back to my initial change.  It's not terribly elegant, but 
it is at least clean, does the job, and passes the tests.

   Just to re-iterate, what I've done is change svn_io_open_unique_file3() 
so that instead of just calling svn_io_open_uniquely_named() it is in fact a 
verbatim copy of svn_io_open_uniquely_named(), except that it uses rand() 
(after a fashion) to generate iterative temporary file names instead of 
counting from 1.

   This time, I believe I have catered for the case where RAND_MAX is only 
32767 (my Windows at least) without being OS-specific.  I have tested it on 
Linux on both ext3 and onto a shared NTFS drive (which used to exhibit the 
fault).

   I have not compiled it for Windows (I don't have a Windows subversion 
build environment), but I have compiled both sides of the ifdef I added.


   I'll include the latest patch here, and put it on the bug entry for 
posterity as well.

   If you're happy with it, great, if not, well, so be it.


[[[
Fix issue #3719: prevent logarithmic slow down when checking out large 
directories onto Windows NTFS

* subversion/libsvn_subr/io.c
   svn_io_open_unique_file3: don't call svn_io_open_uniquely_named(), but 
instead use a copy of that routine that uses rand() to get the next 
potential free number instead if simply incrementing, minimising clashes on 
repeated calls.

]]]

-- 
[neil@fnx ~]# rm -f .signature
[neil@fnx ~]# ls -l .signature
ls: .signature: No such file or directory
[neil@fnx ~]# exit

Re: [PATCH] issue #3719 fix slow large checkouts on Windows

[Stefan Sperling <st...@elego.de>]

On Wed, Mar 02, 2011 at 08:31:01AM +0000, Neil Bird wrote:
> Around about 01/03/11 17:13, Stefan Sperling typed ...
> >Great! Here's a new version that includes a fix for the lock_tests
> >failures. I'll propose this for backport now. Thanks for providing
> >the initial patch submission for this and for helping with testing!
> 
>   Yep, that's the kiddie!  Thanks for the effort;  I'm only sorry I
> couldn't be of any actual help bar testing in the end :)

You've been great help. The fact alone that you started writing a
patch made this issue more likely to get fixed because patch
submissions generally receive more attention than "me too!" problem
reports. This is not because such problem reports aren't being taken
seriously. It's because there's an additional incentive for existing
developers to keep an extra eye on patch submissions. Because that's
how we can grow the team :) So I hope you'll come back some day and
fix another problem, however small it might be, and that you've enjoyed
this exercise even if you don't have any reason to come back.

Re: [PATCH] issue #3719 fix slow large checkouts on Windows

[Neil Bird <ne...@jibbyjobby.co.uk>]

Around about 01/03/11 17:13, Stefan Sperling typed ...
> Great! Here's a new version that includes a fix for the lock_tests
> failures. I'll propose this for backport now. Thanks for providing
> the initial patch submission for this and for helping with testing!

   Yep, that's the kiddie!  Thanks for the effort;  I'm only sorry I 
couldn't be of any actual help bar testing in the end :)

-- 
[neil@fnx ~]# rm -f .signature
[neil@fnx ~]# ls -l .signature
ls: .signature: No such file or directory
[neil@fnx ~]# exit

Re: [PATCH] issue #3719 fix slow large checkouts on Windows

[Stefan Sperling <st...@elego.de>]

On Tue, Mar 01, 2011 at 04:43:47PM +0000, Neil Bird wrote:
> Around about 01/03/11 16:34, Stefan Sperling typed ...
> >It's not perfect yet. There are regression test failures around
> >setting file permissions for svn:needs-lock and svn:executable.
> 
>   Yes, I saw those.
> 
> 
> >But the most interesting bit of information is whether it fixes
> >the performance issues, which I cannot test (no windows box around).
> 
>   Well, it seemed to go OK;  it's taking 8 mins. instead of 85 (but
> that's slower than it should be because both our network and my
> Windows PC are playing up a bit at the moment).
> 
>   It's the same sort of speed I was seeing this morning with my
> noddy patch, certainly.

Great! Here's a new version that includes a fix for the lock_tests
failures. I'll propose this for backport now. Thanks for providing
the initial patch submission for this and for helping with testing!

Re: [PATCH] issue #3719 fix slow large checkouts on Windows

[Neil Bird <ne...@jibbyjobby.co.uk>]

Around about 01/03/11 16:34, Stefan Sperling typed ...
> It's not perfect yet. There are regression test failures around
> setting file permissions for svn:needs-lock and svn:executable.

   Yes, I saw those.


> But the most interesting bit of information is whether it fixes
> the performance issues, which I cannot test (no windows box around).

   Well, it seemed to go OK;  it's taking 8 mins. instead of 85 (but that's 
slower than it should be because both our network and my Windows PC are 
playing up a bit at the moment).

   It's the same sort of speed I was seeing this morning with my noddy 
patch, certainly.

-- 
[neil@fnx ~]# rm -f .signature
[neil@fnx ~]# ls -l .signature
ls: .signature: No such file or directory
[neil@fnx ~]# exit

Re: [PATCH] issue #3719 fix slow large checkouts on Windows

[Stefan Sperling <st...@elego.de>]

On Tue, Mar 01, 2011 at 04:08:30PM +0000, Neil Bird wrote:
> Around about 01/03/11 16:03, Stefan Sperling typed ...
> >Neil, can you try the attached patch, please? Thanks!
> 
>   OK, are you just trying to embarrass me now by getting an
> alternative patch out so quickly??  :-)

Heh. No, that's not the intention :)
 
>   I'll try it now ...

It's not perfect yet. There are regression test failures around
setting file permissions for svn:needs-lock and svn:executable.

But the most interesting bit of information is whether it fixes
the performance issues, which I cannot test (no windows box around).

Re: [PATCH] issue #3719 fix slow large checkouts on Windows

[Neil Bird <ne...@jibbyjobby.co.uk>]

Around about 01/03/11 15:19, Daniel Shahaf typed ...
> The magic is "add 840074 to the old revnum to get the new one":
> http://svn.apache.org/repos/asf/subversion/README

   Ah, all is clear[er].

-- 
[neil@fnx ~]# rm -f .signature
[neil@fnx ~]# ls -l .signature
ls: .signature: No such file or directory
[neil@fnx ~]# exit

Re: [PATCH] issue #3719 fix slow large checkouts on Windows

[Daniel Shahaf <d....@daniel.shahaf.name>]

Quick reply to address just this one point:

Neil Bird wrote on Tue, Mar 01, 2011 at 14:01:34 +0000:
>   All that added to the fact that the revision numbers seem to have
> all changed (the move from Tigris to Apache?), so even when the
> comments *do* say that they're fixing blah from rev. xxx, the xxx is
> wrong.

The magic is "add 840074 to the old revnum to get the new one":
http://svn.apache.org/repos/asf/subversion/README

Re: [PATCH] issue #3719 fix slow large checkouts on Windows

[Neil Bird <ne...@jibbyjobby.co.uk>]

Around about 01/03/11 16:03, Stefan Sperling typed ...
> Neil, can you try the attached patch, please? Thanks!

   OK, are you just trying to embarrass me now by getting an alternative 
patch out so quickly??  :-)

   I'll try it now ...

-- 
[neil@fnx ~]# rm -f .signature
[neil@fnx ~]# ls -l .signature
ls: .signature: No such file or directory
[neil@fnx ~]# exit

Re: [PATCH] issue #3719 fix slow large checkouts on Windows

[Neil Bird <ne...@jibbyjobby.co.uk>]

Around about 01/03/11 16:03, Stefan Sperling typed ...
> Neil, can you try the attached patch, please? Thanks!

Running all tests in lock_tests.py [57/71]...FAILURE
FAIL:  lock_tests.py 25: svn:needs-lock and svn:executable, part I
FAIL:  lock_tests.py 26: svn:needs-lock and svn:executable, part II


   I'm pretty certain that one of my attempts did that.  I'd had to backport 
one of the file perms routines in io.c, from trunk in its entirety to get an 
API that the trunk temp file code wanted, but I couldn't see why it would 
now be failing.

   I think it stuffed up in the routines that get/merge default file 
permissions.


   It does appear to fix the slowdown, though.

-- 
[neil@fnx ~]# rm -f .signature
[neil@fnx ~]# ls -l .signature
ls: .signature: No such file or directory
[neil@fnx ~]# exit

Re: [PATCH] issue #3719 fix slow large checkouts on Windows

[Stefan Sperling <st...@elego.de>]

On Tue, Mar 01, 2011 at 04:42:28PM +0100, Stefan Sperling wrote:
> I will try backporting the trunk code to 1.6.x myself.
> If that gets anywhere then we can use it.

Neil, can you try the attached patch, please? Thanks!

Re: [PATCH] issue #3719 fix slow large checkouts on Windows

[Stefan Sperling <st...@elego.de>]

On Tue, Mar 01, 2011 at 02:01:34PM +0000, Neil Bird wrote:
> Around about 08/02/11 10:01, Neil Bird typed ...
>   Just to re-iterate, what I've done is change
> svn_io_open_unique_file3() so that instead of just calling
> svn_io_open_uniquely_named() it is in fact a verbatim copy of
> svn_io_open_uniquely_named(), except that it uses rand() (after a
> fashion) to generate iterative temporary file names instead of
> counting from 1.

The problem with this approach is that is also changes the numbers
for temporary files used to store log messages. These temporary
files are part of the user interface, in a way.
I think that using random numbers in those files names might confuse people.
And we're generally trying to avoid making such changes in a patch release.

When there are several svn-commit.tmp files, I find it quite convenient
to be able to tell from the filename which file is the latest.

>   I'll include the latest patch here, and put it on the bug entry
> for posterity as well.
> 
>   If you're happy with it, great, if not, well, so be it.

Thanks for trying!

I will try backporting the trunk code to 1.6.x myself.
If that gets anywhere then we can use it.

Else, we could use a slightly modified version of your patch 
that keeps the existing behaviour for log message temp files.