Re: [PATCH] git-svn: Make it scream by minimizing temp files

[Marcus Griep <marcus@griep.us>]

Eric Wong wrote:
> 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'.

>> +		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.

>> +	} else {
>> +		$TEMP_FILES{$fd} = File::Temp->new(
>> +									TEMPLATE => 'GitSvn_XXXXXX',
>> +									DIR => File::Spec->tmpdir
>> +									) or croak $!;
> 
> Way too much indentation :x

That's what I get for assuming a tab width of 4.  I'll redo it with
about half as many tabs.

>> +		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
את.ψο´