From: Junio C Hamano <hidden> Date: 2016-06-15 22:58:10
Mark Levedahl [off-list ref] writes:
quoted
In order to limit the adverse effects caused by this implementation,
we provide a new "fast stat" interface, which allows us to use this
only for interactions with the index (i.e. the cached stat data).
Signed-off-by: Ramsay Jones <redacted>
---
I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results
using your prior patch (removing the Cygwin specific lstat entirely)
and get the same results with both, so this seems ok from me.
My comparison point was created by reverting your current patch from
pu, then reapplying your earlier patch on top, so the only difference
was which approach was used to address the stat functions.
Caveats:
1) I don't find any speed improvement of the current patch over the
previous one (the tests actually ran faster with the earlier patch,
though the difference was less than 1%).
2) I still question this whole approach, especially having this
non-POSIX compliant mode be the default. Running in this mode breaks
interoperability with Linux, but providing a Linux environment is the
*primary* goal of Cygwin.
Sounds like we are better off without this patch, and instead remove
the "schizophrenic stat"? I do not have a strong opinion either
way, except that I tend to agree with your point 2) above.
In order to limit the adverse effects caused by this implementation,
we provide a new "fast stat" interface, which allows us to use this
only for interactions with the index (i.e. the cached stat data).
Signed-off-by: Ramsay Jones <redacted>
---
I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results
using your prior patch (removing the Cygwin specific lstat entirely)
and get the same results with both, so this seems ok from me.
My comparison point was created by reverting your current patch from
pu, then reapplying your earlier patch on top, so the only difference
was which approach was used to address the stat functions.
Caveats:
1) I don't find any speed improvement of the current patch over the
previous one (the tests actually ran faster with the earlier patch,
though the difference was less than 1%).
Hm, measuring the time for the test suite is one thing,
did you measure the time of "git status" with and without the patch?
(I don't have my test system at hand, so I can test in a few days/weeks)
quoted
2) I still question this whole approach, especially having this
non-POSIX compliant mode be the default. Running in this mode breaks
interoperability with Linux, but providing a Linux environment is the
*primary* goal of Cygwin.
Sounds like we are better off without this patch, and instead remove
the "schizophrenic stat"? I do not have a strong opinion either
way, except that I tend to agree with your point 2) above.
My understanding is that we want both:
Introduction of fast_lstat() as phase 1,
and the removal of the "schizophrenic stat" in compat/cygwin.c
as phase 2. (or do I missunderstand something ?)
And yes, phase 3:
The day we have a both reliable and fast
lstat() in cygwin, we can remove compat/cygwin.[ch]
From: Mark Levedahl <hidden> Date: 2016-06-15 22:58:11
On 07/15/2013 03:49 PM, Junio C Hamano wrote:
Mark Levedahl [off-list ref] writes:
quoted
quoted
In order to limit the adverse effects caused by this implementation,
we provide a new "fast stat" interface, which allows us to use this
only for interactions with the index (i.e. the cached stat data).
Signed-off-by: Ramsay Jones <redacted>
---
I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results
using your prior patch (removing the Cygwin specific lstat entirely)
and get the same results with both, so this seems ok from me.
My comparison point was created by reverting your current patch from
pu, then reapplying your earlier patch on top, so the only difference
was which approach was used to address the stat functions.
Caveats:
1) I don't find any speed improvement of the current patch over the
previous one (the tests actually ran faster with the earlier patch,
though the difference was less than 1%).
2) I still question this whole approach, especially having this
non-POSIX compliant mode be the default. Running in this mode breaks
interoperability with Linux, but providing a Linux environment is the
*primary* goal of Cygwin.
Sounds like we are better off without this patch, and instead remove
the "schizophrenic stat"? I do not have a strong opinion either
way, except that I tend to agree with your point 2) above.
In case my opinion is unclear, I think removal of the schizophrenic stat
is the right approach. Speed is important, but not at the expense of
correctness.
Mark
From: Mark Levedahl <hidden> Date: 2016-06-15 22:58:11
On 07/15/2013 10:06 PM, Torsten Bögershausen wrote:
On 2013-07-15 21.49, Junio C Hamano wrote:
quoted
Mark Levedahl [off-list ref] writes:
quoted
quoted
In order to limit the adverse effects caused by this implementation,
we provide a new "fast stat" interface, which allows us to use this
only for interactions with the index (i.e. the cached stat data).
Signed-off-by: Ramsay Jones <redacted>
---
I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results
using your prior patch (removing the Cygwin specific lstat entirely)
and get the same results with both, so this seems ok from me.
My comparison point was created by reverting your current patch from
pu, then reapplying your earlier patch on top, so the only difference
was which approach was used to address the stat functions.
Caveats:
1) I don't find any speed improvement of the current patch over the
previous one (the tests actually ran faster with the earlier patch,
though the difference was less than 1%).
Hm, measuring the time for the test suite is one thing,
did you measure the time of "git status" with and without the patch?
(I don't have my test system at hand, so I can test in a few days/weeks)
Timing for 5 rounds of "git status" in the git project. First, with the
current fast_lstat patches:
/usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done
real 0m0.218s
user 0m0.000s
sys 0m0.218s
real 0m0.187s
user 0m0.077s
sys 0m0.109s
real 0m0.187s
user 0m0.030s
sys 0m0.156s
real 0m0.203s
user 0m0.031s
sys 0m0.171s
real 0m0.187s
user 0m0.062s
sys 0m0.124s
Now, with Ramsay's original patch just removing the non-Posix stat
functions:
/usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done
real 0m0.218s
user 0m0.046s
sys 0m0.171s
real 0m0.187s
user 0m0.015s
sys 0m0.171s
real 0m0.187s
user 0m0.015s
sys 0m0.171s
real 0m0.187s
user 0m0.047s
sys 0m0.140s
real 0m0.187s
user 0m0.031s
sys 0m0.156s
I see no difference in the above. (Yes, I checked multiple times that I
was using different executables).
quoted
quoted
2) I still question this whole approach, especially having this
non-POSIX compliant mode be the default. Running in this mode breaks
interoperability with Linux, but providing a Linux environment is the
*primary* goal of Cygwin.
Sounds like we are better off without this patch, and instead remove
the "schizophrenic stat"? I do not have a strong opinion either
way, except that I tend to agree with your point 2) above.
My understanding is that we want both:
Introduction of fast_lstat() as phase 1,
and the removal of the "schizophrenic stat" in compat/cygwin.c
as phase 2. (or do I missunderstand something ?)
And yes, phase 3:
The day we have a both reliable and fast
lstat() in cygwin, we can remove compat/cygwin.[ch]
If you know how to make Cygwin's stat faster while maintaining Posix
semantics, please post a patch to the Cygwin list, they would *love* it.
Mark
On Tue, Jul 16, 2013 at 7:54 AM, Mark Levedahl [off-list ref] wrote:
I see no difference in the above. (Yes, I checked multiple times that I was
using different executables).
Are you sure that you set core.filemode to false before testing?
If you have core.filemode set to true then you _always_ use Cygwin stat,
so it does not make any difference for you.
Dmitry
From: Ramsay Jones <hidden> Date: 2016-06-15 22:58:12
Mark Levedahl wrote:
On 07/15/2013 10:06 PM, Torsten Bögershausen wrote:
quoted
On 2013-07-15 21.49, Junio C Hamano wrote:
quoted
Mark Levedahl [off-list ref] writes:
quoted
quoted
In order to limit the adverse effects caused by this implementation,
we provide a new "fast stat" interface, which allows us to use this
only for interactions with the index (i.e. the cached stat data).
Signed-off-by: Ramsay Jones <redacted>
---
I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results
using your prior patch (removing the Cygwin specific lstat entirely)
and get the same results with both, so this seems ok from me.
My comparison point was created by reverting your current patch from
pu, then reapplying your earlier patch on top, so the only difference
was which approach was used to address the stat functions.
Caveats:
1) I don't find any speed improvement of the current patch over the
previous one (the tests actually ran faster with the earlier patch,
though the difference was less than 1%).
Hm, measuring the time for the test suite is one thing,
did you measure the time of "git status" with and without the patch?
(I don't have my test system at hand, so I can test in a few days/weeks)
Timing for 5 rounds of "git status" in the git project. First, with the
current fast_lstat patches:
/usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done
real 0m0.218s
user 0m0.000s
sys 0m0.218s
real 0m0.187s
user 0m0.077s
sys 0m0.109s
real 0m0.187s
user 0m0.030s
sys 0m0.156s
real 0m0.203s
user 0m0.031s
sys 0m0.171s
real 0m0.187s
user 0m0.062s
sys 0m0.124s
Now, with Ramsay's original patch just removing the non-Posix stat
functions:
/usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done
real 0m0.218s
user 0m0.046s
sys 0m0.171s
real 0m0.187s
user 0m0.015s
sys 0m0.171s
real 0m0.187s
user 0m0.015s
sys 0m0.171s
real 0m0.187s
user 0m0.047s
sys 0m0.140s
real 0m0.187s
user 0m0.031s
sys 0m0.156s
I see no difference in the above. (Yes, I checked multiple times that I
was using different executables).
Hmm, that looks good. :-D
Torsten reported a performance boost using the win32 stat() implementation
on a linux git repo (2s -> 1s, if I recall correctly) on cygwin 1.7.
Do you have a larger repo available to test?
If performance isn't an issue (it isn't for _me_), then I will happily
re-submit my original patch (removing the win32 stat implementation).
[Hmm, I may do anyway!]
ATB,
Ramsay Jones
On 07/15/2013 10:06 PM, Torsten Bögershausen wrote:
quoted
On 2013-07-15 21.49, Junio C Hamano wrote:
quoted
Mark Levedahl [off-list ref] writes:
quoted
quoted
In order to limit the adverse effects caused by this implementation,
we provide a new "fast stat" interface, which allows us to use this
only for interactions with the index (i.e. the cached stat data).
Signed-off-by: Ramsay Jones <redacted>
---
I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results
using your prior patch (removing the Cygwin specific lstat entirely)
and get the same results with both, so this seems ok from me.
My comparison point was created by reverting your current patch from
pu, then reapplying your earlier patch on top, so the only difference
was which approach was used to address the stat functions.
Caveats:
1) I don't find any speed improvement of the current patch over the
previous one (the tests actually ran faster with the earlier patch,
though the difference was less than 1%).
Hm, measuring the time for the test suite is one thing,
did you measure the time of "git status" with and without the patch?
(I don't have my test system at hand, so I can test in a few days/weeks)
Timing for 5 rounds of "git status" in the git project. First, with the
current fast_lstat patches:
/usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done
real 0m0.218s
user 0m0.000s
sys 0m0.218s
real 0m0.187s
user 0m0.077s
sys 0m0.109s
real 0m0.187s
user 0m0.030s
sys 0m0.156s
real 0m0.203s
user 0m0.031s
sys 0m0.171s
real 0m0.187s
user 0m0.062s
sys 0m0.124s
Now, with Ramsay's original patch just removing the non-Posix stat
functions:
/usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done
real 0m0.218s
user 0m0.046s
sys 0m0.171s
real 0m0.187s
user 0m0.015s
sys 0m0.171s
real 0m0.187s
user 0m0.015s
sys 0m0.171s
real 0m0.187s
user 0m0.047s
sys 0m0.140s
real 0m0.187s
user 0m0.031s
sys 0m0.156s
I see no difference in the above. (Yes, I checked multiple times that I
was using different executables).
Hmm, that looks good. :-D
Torsten reported a performance boost using the win32 stat() implementation
on a linux git repo (2s -> 1s, if I recall correctly) on cygwin 1.7.
Do you have a larger repo available to test?
(I have a 5 years old Dual Core, 2.5 Ghz, 1 TB hard disk, Win XP, cygwin 1.7)
On that machine I can see the performance boost.
Which kind of computers are you guys using?
SSD/hard disk ?
How much RAM ?
Which OS ?
Is there a difference between Win XP, Win7, Win8?
[snip]
From: Mark Levedahl <hidden> Date: 2016-06-15 22:58:12
On 07/18/2013 05:49 PM, Torsten Bögershausen wrote:
On 2013-07-18 19.50, Ramsay Jones wrote:
quoted
Hmm, that looks good. :-D
Torsten reported a performance boost using the win32 stat() implementation
on a linux git repo (2s -> 1s, if I recall correctly) on cygwin 1.7.
Do you have a larger repo available to test?
(I have a 5 years old Dual Core, 2.5 Ghz, 1 TB hard disk, Win XP, cygwin 1.7)
On that machine I can see the performance boost.
Which kind of computers are you guys using?
SSD/hard disk ?
How much RAM ?
Which OS ?
Is there a difference between Win XP, Win7, Win8?
[snip]
My previous results were from a Win 7 laptop, 2.7 GHz 2nd generation I7,
8 Gig Ram, 250 GByte spinning rust drive, all formatted NTFS.
Here's some more results, running WinXP in VirtualBox on my older Linux
laptop (2.5 GHz Penryn dual core, 500 GByte spinning rust, virtual file
system is NTFS). First, results using Ramsay's last patch on pu adding
the fast_lstat: Timing results are after first doing 5 'git status runs'
to assure the cache is hot:
% using the fast_lstat and friends...
/usr/local/src/git>time git -c core.filemode=false status >& /dev/null
real 0m0.469s
user 0m0.062s
sys 0m0.436s
/usr/local/src/git>
/usr/local/src/git>time git -c core.filemode=true status >& /dev/null
real 0m0.719s
user 0m0.030s
sys 0m0.686s
/usr/local/src/git>
And now the same. but using Ramsay's first patch that removes all win32
stat stuff and forces everything to go through Cygwin's normal stat/fstat:
% stat - with / without core.filemode, no win32 stats
/usr/local/src/git>time git -c core.filemode=false status >& /dev/null
real 0m0.328s
user 0m0.093s
sys 0m0.264s
/usr/local/src/git>
/usr/local/src/git>time git -c core.filemode=true status >& /dev/null
real 0m0.625s
user 0m0.124s
sys 0m0.500s
/usr/local/src/git>
Unlike the results on the fast Win7 laptop, the above show statistically
significant slow down from the fast_lstat approach. I'm just not seeing
a case for the special case handling, and of course Junio has already
voted with his preference of removing the special case stuff as well.
Mark