Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Marius Storm-Olsen <marius@trolltech.com>]

[-- Attachment #1: Type: text/plain, Size: 4943 bytes --]

Johannes Sixt wrote:
> On Sunday 02 September 2007 16:51, Marius Storm-Olsen wrote:
>> This gives us a significant speedup when adding, committing and stat'ing
>> files. (Also, since Windows doesn't really handle symlinks, it's fine that
>> stat just uses lstat)
>>
>> Signed-off-by: Marius Storm-Olsen <mstormo_git@storm-olsen.com>
> 
> Your numbers show an improvement of 50% and more. That is terrific!

Yes, I was surprised myself about the impact. Didn't think it would make
_such_ a difference. And if you compare it to the results we had before
Linus' performance fix too, just checkout the performance improvements
we've had (based on Moe's test script):

    Before                        Now
    -------------------------     -------------------------
    Command: git init             Command: git init
    -------------------------     -------------------------
    real    0m0.031s              real       0m0.078s
    user    0m0.031s              user       0m0.031s
    sys     0m0.000s              sys        0m0.000s
    -------------------------     -------------------------
    Command: git add .            Command: git add .
    -------------------------     -------------------------
    real    0m19.328s             real       0m12.187s
    user    0m0.015s              user       0m0.015s
    sys     0m0.015s              sys        0m0.015s
    -------------------------     -------------------------
    Command: git commit -a...     Command: git commit -a...
    -------------------------     -------------------------
    real    0m30.937s             real       0m17.297s
    user    0m0.015s              user       0m0.015s
    sys     0m0.015s              sys        0m0.015s
    -------------------------     -------------------------
    3x Command: git-status        3x Command: git-status
    -------------------------     -------------------------
    real    0m19.531s             real       0m5.344s
    user    0m0.211s              user       0m0.015s
    sys     0m0.136s              sys        0m0.031s

    real    0m19.532s             real       0m5.390s
    user    0m0.259s              user       0m0.031s
    sys     0m0.091s              sys        0m0.000s

    real    0m19.593s             real       0m5.344s
    user    0m0.211s              user       0m0.015s
    sys     0m0.152s              sys        0m0.016s
    -------------------------     -------------------------
    Command: git commit...        Command: git commit...
             (single file)                 (single file)
    -------------------------     -------------------------
    real    0m36.688s             real       0m7.875s
    user    0m0.031s              user       0m0.015s
    sys     0m0.000s              sys        0m0.000s

> I'll test it out an put the patch into mingw.git. I hope you don't mind if I 
> also include your analysis and statistics in the commit message. It's worth 
> keeping around! BTW, which of your email addresses would you like registered 
> as author?

Sure, include the stats if you'd like. You can use
mstormo_git@storm-olsen.com for email address.

>> +		ext = strrchr(file_name, '.');
>> +		if (ext && (!_stricmp(ext, ".exe") ||
>> +			    !_stricmp(ext, ".com") ||
>> +			    !_stricmp(ext, ".bat") ||
>> +			    !_stricmp(ext, ".cmd")))
>> +			fMode |= S_IEXEC;
>> +		}
> 
> I'm slightly negative about this. For a native Windows project the executable 
> bit does not matter, and for a cross-platform project this check is not 
> sufficient, but can even become annoying (think of a file 
> named 'www.google.com'). So we can just as well spare the few cycles.

Ok, that's fine by me. It was only added for completeness, and with no
benefits I'd say we drop it too.

>> +		buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since
>> it's not a stat64 */
> 
> Here's an idea for the future: With this self-made stat() implementation it 
> should also be possible to get rid of Windows's native struct stat: Make a 
> private definition of it, too, and use all 64 bits.

Yep, that will shave off one assignment, bit-shift and addition. Quick
operations, but still worth while IMO. No point wasting cycles where we
don't have to.

>>  		return 0;
>> +	}
>> +	errno = ENOENT;
> 
> Of course we need a bit more detailed error conditions, most importantly 
> EACCES should be distinguished.

Right, you want to do that in a second commit?

>> +/* Make git on Windows use git_lstat and git_stat instead of lstat and
>> stat */ +int git_lstat(const char *file_name, struct stat *buf);
>> +int git_stat(const char *file_name, struct stat *buf);
>> +#define lstat(x,y) git_lstat(x,y)
>> +#define stat(x,y) git_stat(x,y)
> 
> I'd go the short route without git_stat() and
> 
> #define stat(x,y) git_lstat(x,y)

Please do, thanks.

--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]