Re: [PATCH] git-svn: Make it scream by minimizing temp files
- From
Marcus Griep <marcus@griep.us>
- Date
- Aug 9, 2008, 15:45 UTC
- Message-ID
- <489DBB8A.2060207@griep.us>
- In-Reply-To
- <20080809062521.GA10480@untitled>
Eric Wong wrote:
Show 6 quoted lines
> Wow. I've considered this in the past didn't think there would be a > significant difference (of course I'm always network I/O bound). Which > platform and filesystem are you using are you using for tests? > > I don't notice any difference running the test suite on Linux + ext3 > here, but the test suite is not a good benchmark :)
Yeah, much of the test suite uses small repositories without much history. Where you see the benefit is with large repositories with many files. In such cases, even a small speedup can reduce the total import time significantly.
My benchmark against a large repository uses the svn we have at work, but there is currently a planned power outage, so I'll have to wait until tonight to run my benchmarks there (and they'll take significant time).
Nonetheless, my tests against the smaller Boo repository showed almost no change in user time, but a 10% reduction in system time used. There was also a small (1%) drop in minor page faults. I'm confident in these results, but won't certify them until I'm able to run the tests on a much larger repository.
>> +use File::Temp qw/ :seekable /; > > qw/ :seekable / does not appear in my version of Perl (5.8.8-7etch3 from > Debian stable) Just having "use File::Temp;" there works for me.
My newbishness in perl shows. I'll change it to a simple 'use'.
Show 8 quoted lines
>> + seek $TEMP_FILES{$fd}, 0, 0 or croak $!;
>
> Perhaps a sysseek in addition to the seek above would help
> with the problems you mentioned in the other email.
>
> sysseek $TEMP_FILES{$fd}, 0, 0 or croak $!;
>
> (It doesn't seem to affect me when running the test suite, though).Sounds like a good idea, but I found the source of my cygwin issue, namely that /tmp (which perl uses for its temp files) was mounted in textmode. I fixed that by remounting that folder in binmode.
Nonetheless, if consumers may use sysread, after getting the file handle then we'll want to use sysseek.
Show 7 quoted lines
>> + } else {
>> + $TEMP_FILES{$fd} = File::Temp->new(
>> + TEMPLATE => 'GitSvn_XXXXXX',
>> + DIR => File::Spec->tmpdir
>> + ) or croak $!;
>
> Way too much indentation :xThat's what I get for assuming a tab width of 4. I'll redo it with about half as many tabs.
Show 7 quoted lines
>> + if (defined $autoflush) {
>> + $TEMP_FILES{$fd}->autoflush($autoflush);
>> + }
>
> Given how much we interact with external programs, I'd rather force
> every autoflush on every file handle to avoid subtle bugs on
> different platforms. It's faster in some (most?) cases, too.That sounds good to me.
> Also, this seems generic enough that other programs (git-cvsimport > perhaps) can probably use it, too. So maybe it could go into Git.pm or > a new module, Git/Tempfile.pm?
I'd advocate the latter since it's not really Git functionality, but rather a support, so a submodule would perhaps be the better placement.
Also, I came up with one more optimization inside 'sub close_file', so I'll roll that in too. Tell me where you/the community would prefer the tempfile functionality, and I'll submit a new patch series with one patch for the module and one patch for git-svn.
By then, I should have some better benchmark results.
-- Marcus Griep GPG Key ID: 0x5E968152 —— http://www.boohaunt.net את.ψο´