Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
On 07/18/2013 07:32 PM, Junio C Hamano wrote: Mark Levedahl mleved...@gmail.com writes: 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. Please don't take what I said as any vote in this thread. I do not have a first-hand data to back anything up. I was primarily trying to see my understanding of the consensus of the thread was correct. If we can do without s/lstat/fast_lstat/ almost everywhere in the codebase, of course, I would be happier, as it would give us one less thing to worry about. If the assumptions like they were declining minority and only lose population over time, it is easy for them to revert the removal and keep going, and removal will not hurt them too much in the first place, only a few hundred milliseconds, that might trump the longer-term maintainability issue, and we may end up having to carry that win32 stat implementation a bit longer until these users all switch to Cygwin 1.7, but judging from the cvs binary seems to be built incorrectly incident the other day, it might be the case some users still hesitate to update, fearing that 1.7 series may not be solid enough, perhaps? I cannot say how many users of 1.5 exist. I see no evidence of 1.5 users on the Cygwin lists, the developers noted a total of 14 downloads of the 1.5 installer in the year prior to removal of 1.5 from the mirrors. The stated reason for keeping 1.5 available for four years after its development stopped was support of older Windows variants (which Microsoft dropped support of before Cygwin did, BTW). But, none of this is conclusive about the current relevance of v 1.5. The status as I understand things: 1) The existing schizophrenic stat on master is incompatible with the new reference api on pu, therefore some change is required. 2) Ramsay has graciously provided two separate patches to address the above, one reverting to use only of cygwin stat/lstat, one including a fast_lstat that should provide better speed at the expense of POSIX compliance. 3) We have conflicting reports about the speed of the second patch: Ramsay shows a good speed up on Cygwin 1.5, with slight performance regrets on MINGW, no change on Linux. I found no effect on a current bare-metal Window 7 installation using Cygwin 1.7, but degradation on a virtualized WinXP installation using Cygwin 1.7. Ramsay also showed a significant performance difference between running from the git tree vs being installed, I looked for this effect but failed to replicate it. The maintenance argument between the two patches is clear, the performance argument less so. Perhaps others can help clarify this. Mark -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
Mark Levedahl mleved...@gmail.com writes: Cygwin 1.7 is very different than the earlier, no longer supported, and no longer available Cygwin variants in many ways, but stat is one of them. Cygwin 1.7 uses Windows ACLs to represent file permissions, and therefore gets the file permissions directly from the underlying OS calls. Earlier Cygwin versions (attempted to) overlay POSIX permissions on Windows systems using extended attributes and other means, and in many cases had to resort to opening the file and examining it to determine executability. This is not true in 1.7. Therefore, your later patch would be expected to have much less benefit for 1.7 than for 1.5 (I don't detect *any* benefit on 1.7 when I set core.filemode=false). There are many choices, three are: a) Remove the win32 stat funcs, eliminating all of the troublesome code paths and maintenance burden (your original patch). b) Add your latest patch, with attendant complexity and maintenance burden, to support a version of Cygwin that is no longer available and was last updated over four years ago. c) Like b, except make this triggered only by a CYGWIN_15 macro, limiting this to use by the legacy cygwin platform. Let's do (a) in a single patch, then. People who do want to keep running older Cygwin installation they already have can revert the removal and rebuild Git, but the number of people who have to do so will become only smaller over time if older Cygwin versions are no longer available. I presume that we _could_ add a CYGWIN_15 macro that conditionally keeps the win32 lstat implementation and get_st_mode_bits() part, and that might make it easier for folks with older Cygwin installations, but I am not sure if it is worth it. I strongly vote for a, could support c, but fear b is just going to keep us chasing down bugs. Especially so when we consider that this patch can only speed things up when core.filemode=false, which mode: a) causes git to fail its test suite. b) breaks compatibility with Linux c) violates the primary goal of the Cygwin project, which is to provide a Linux environment on Windows. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
Mark Levedahl wrote:
On 07/15/2013 10:06 PM, Torsten Bögershausen wrote:
On 2013-07-15 21.49, Junio C Hamano wrote:
Mark Levedahl mleved...@gmail.com writes:
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 ram...@ramsay1.demon.co.uk
---
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/gitfor i in {1..5} ; do time git status /dev/null ; done
real0m0.218s
user0m0.000s
sys 0m0.218s
real0m0.187s
user0m0.077s
sys 0m0.109s
real0m0.187s
user0m0.030s
sys 0m0.156s
real0m0.203s
user0m0.031s
sys 0m0.171s
real0m0.187s
user0m0.062s
sys 0m0.124s
Now, with Ramsay's original patch just removing the non-Posix stat
functions:
/usr/local/src/gitfor i in {1..5} ; do time git status /dev/null ; done
real0m0.218s
user0m0.046s
sys 0m0.171s
real0m0.187s
user0m0.015s
sys 0m0.171s
real0m0.187s
user0m0.015s
sys 0m0.171s
real0m0.187s
user0m0.047s
sys 0m0.140s
real0m0.187s
user0m0.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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
On 2013-07-18 19.50, Ramsay Jones wrote:
Mark Levedahl wrote:
On 07/15/2013 10:06 PM, Torsten Bögershausen wrote:
On 2013-07-15 21.49, Junio C Hamano wrote:
Mark Levedahl mleved...@gmail.com writes:
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 ram...@ramsay1.demon.co.uk
---
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/gitfor i in {1..5} ; do time git status /dev/null ; done
real0m0.218s
user0m0.000s
sys 0m0.218s
real0m0.187s
user0m0.077s
sys 0m0.109s
real0m0.187s
user0m0.030s
sys 0m0.156s
real0m0.203s
user0m0.031s
sys 0m0.171s
real0m0.187s
user0m0.062s
sys 0m0.124s
Now, with Ramsay's original patch just removing the non-Posix stat
functions:
/usr/local/src/gitfor i in {1..5} ; do time git status /dev/null ; done
real0m0.218s
user0m0.046s
sys 0m0.171s
real0m0.187s
user0m0.015s
sys 0m0.171s
real0m0.187s
user0m0.015s
sys 0m0.171s
real0m0.187s
user0m0.047s
sys 0m0.140s
real0m0.187s
user0m0.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]
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
On 07/18/2013 05:49 PM, Torsten Bögershausen wrote: On 2013-07-18 19.50, Ramsay Jones wrote: 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/gittime git -c core.filemode=false status /dev/null real0m0.469s user0m0.062s sys 0m0.436s /usr/local/src/git /usr/local/src/gittime git -c core.filemode=true status /dev/null real0m0.719s user0m0.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/gittime git -c core.filemode=false status /dev/null real0m0.328s user0m0.093s sys 0m0.264s /usr/local/src/git /usr/local/src/gittime git -c core.filemode=true status /dev/null real0m0.625s user0m0.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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
Mark Levedahl mleved...@gmail.com writes: 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. Please don't take what I said as any vote in this thread. I do not have a first-hand data to back anything up. I was primarily trying to see my understanding of the consensus of the thread was correct. If we can do without s/lstat/fast_lstat/ almost everywhere in the codebase, of course, I would be happier, as it would give us one less thing to worry about. If the assumptions like they were declining minority and only lose population over time, it is easy for them to revert the removal and keep going, and removal will not hurt them too much in the first place, only a few hundred milliseconds, that might trump the longer-term maintainability issue, and we may end up having to carry that win32 stat implementation a bit longer until these users all switch to Cygwin 1.7, but judging from the cvs binary seems to be built incorrectly incident the other day, it might be the case some users still hesitate to update, fearing that 1.7 series may not be solid enough, perhaps? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
On Tue, Jul 16, 2013 at 7:54 AM, Mark Levedahl mleved...@gmail.com 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
Mark Levedahl wrote: On 07/10/2013 04:23 PM, Ramsay Jones wrote: Commit adbc0b6b (cygwin: Use native Win32 API for stat, 30-09-2008) added a Win32 specific implementation of the stat functions. In order to handle absolute paths, cygwin mount points and symbolic links, this implementation may fall back on the standard cygwin l/stat() functions. Also, the choice of cygwin or Win32 functions is made lazily (by the first call(s) to l/stat) based on the state of some config variables. Unfortunately, this schizophrenic stat implementation has been the source of many problems ever since. For example, see commits 7faee6b8, 79748439, 452993c2, 085479e7, b8a97333, 924aaf3e, 05bab3ea and 0117c2f0. 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 ram...@ramsay1.demon.co.uk --- 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. Thank you for testing this. 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. Yes, I agree. Since I _always_ disable the Win32 stat functions (by setting core.filemode by hand - yes it's a little annoying), I don't get any benefit from the added complexity. However, I don't use git on cygwin with *large* repositories. git.git is pretty much as large as it gets. So, the difference in performance for me amounts to something like 0.120s - 0.260s, which I can barely notice. Other people may not be so lucky ... I would be happy if my original patch (removing the win32 stat funcs) was applied, but others may not be. :-P ATB, Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
On 07/16/2013 11:42 AM, Dmitry Potapov wrote: On Tue, Jul 16, 2013 at 7:54 AM, Mark Levedahl mleved...@gmail.com 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? yes. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
On 07/16/2013 05:36 PM, Ramsay Jones wrote: 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. Yes, I agree. Since I _always_ disable the Win32 stat functions (by setting core.filemode by hand - yes it's a little annoying), I don't get any benefit from the added complexity. However, I don't use git on cygwin with *large* repositories. git.git is pretty much as large as it gets. So, the difference in performance for me amounts to something like 0.120s - 0.260s, which I can barely notice. Other people may not be so lucky ... I would be happy if my original patch (removing the win32 stat funcs) was applied, but others may not be. :-P ATB, Ramsay Jones Cygwin 1.7 is very different than the earlier, no longer supported, and no longer available Cygwin variants in many ways, but stat is one of them. Cygwin 1.7 uses Windows ACLs to represent file permissions, and therefore gets the file permissions directly from the underlying OS calls. Earlier Cygwin versions (attempted to) overlay POSIX permissions on Windows systems using extended attributes and other means, and in many cases had to resort to opening the file and examining it to determine executability. This is not true in 1.7. Therefore, your later patch would be expected to have much less benefit for 1.7 than for 1.5 (I don't detect *any* benefit on 1.7 when I set core.filemode=false). There are many choices, three are: a) Remove the win32 stat funcs, eliminating all of the troublesome code paths and maintenance burden (your original patch). b) Add your latest patch, with attendant complexity and maintenance burden, to support a version of Cygwin that is no longer available and was last updated over four years ago. c) Like b, except make this triggered only by a CYGWIN_15 macro, limiting this to use by the legacy cygwin platform. I strongly vote for a, could support c, but fear b is just going to keep us chasing down bugs. Especially so when we consider that this patch can only speed things up when core.filemode=false, which mode: a) causes git to fail its test suite. b) breaks compatibility with Linux c) violates the primary goal of the Cygwin project, which is to provide a Linux environment on Windows. Mark -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
Mark Levedahl mleved...@gmail.com writes: 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 ram...@ramsay1.demon.co.uk --- 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
On 2013-07-15 21.49, Junio C Hamano wrote: Mark Levedahl mleved...@gmail.com writes: 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 ram...@ramsay1.demon.co.uk --- 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) 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] -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
On 07/15/2013 03:49 PM, Junio C Hamano wrote: Mark Levedahl mleved...@gmail.com writes: 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 ram...@ramsay1.demon.co.uk --- 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
On 07/15/2013 10:06 PM, Torsten Bögershausen wrote:
On 2013-07-15 21.49, Junio C Hamano wrote:
Mark Levedahl mleved...@gmail.com writes:
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 ram...@ramsay1.demon.co.uk
---
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/gitfor i in {1..5} ; do time git status /dev/null ; done
real0m0.218s
user0m0.000s
sys 0m0.218s
real0m0.187s
user0m0.077s
sys 0m0.109s
real0m0.187s
user0m0.030s
sys 0m0.156s
real0m0.203s
user0m0.031s
sys 0m0.171s
real0m0.187s
user0m0.062s
sys 0m0.124s
Now, with Ramsay's original patch just removing the non-Posix stat
functions:
/usr/local/src/gitfor i in {1..5} ; do time git status /dev/null ; done
real0m0.218s
user0m0.046s
sys 0m0.171s
real0m0.187s
user0m0.015s
sys 0m0.171s
real0m0.187s
user0m0.015s
sys 0m0.171s
real0m0.187s
user0m0.047s
sys 0m0.140s
real0m0.187s
user0m0.031s
sys 0m0.156s
I see no difference in the above. (Yes, I checked multiple times that I
was using different executables).
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
On 07/10/2013 04:23 PM, Ramsay Jones wrote: Commit adbc0b6b (cygwin: Use native Win32 API for stat, 30-09-2008) added a Win32 specific implementation of the stat functions. In order to handle absolute paths, cygwin mount points and symbolic links, this implementation may fall back on the standard cygwin l/stat() functions. Also, the choice of cygwin or Win32 functions is made lazily (by the first call(s) to l/stat) based on the state of some config variables. Unfortunately, this schizophrenic stat implementation has been the source of many problems ever since. For example, see commits 7faee6b8, 79748439, 452993c2, 085479e7, b8a97333, 924aaf3e, 05bab3ea and 0117c2f0. 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 ram...@ramsay1.demon.co.uk --- 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. Mark -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions
Commit adbc0b6b (cygwin: Use native Win32 API for stat, 30-09-2008)
added a Win32 specific implementation of the stat functions. In order
to handle absolute paths, cygwin mount points and symbolic links, this
implementation may fall back on the standard cygwin l/stat() functions.
Also, the choice of cygwin or Win32 functions is made lazily (by the
first call(s) to l/stat) based on the state of some config variables.
Unfortunately, this schizophrenic stat implementation has been the
source of many problems ever since. For example, see commits 7faee6b8,
79748439, 452993c2, 085479e7, b8a97333, 924aaf3e, 05bab3ea and 0117c2f0.
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 ram...@ramsay1.demon.co.uk
---
builtin/apply.c| 8
builtin/commit.c | 2 +-
builtin/ls-files.c | 2 +-
builtin/rm.c | 2 +-
builtin/update-index.c | 2 +-
check-racy.c | 2 +-
compat/cygwin.c| 48 ++--
compat/cygwin.h| 17 +
diff-lib.c | 2 +-
diff.c | 2 +-
entry.c| 6 +++---
git-compat-util.h | 27 +--
help.c | 5 +
path.c | 9 +
preload-index.c| 2 +-
read-cache.c | 6 +++---
unpack-trees.c | 8
17 files changed, 68 insertions(+), 82 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 0e9b631..f5046a6 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3091,7 +3091,7 @@ static int checkout_target(struct cache_entry *ce, struct
stat *st)
memset(costate, 0, sizeof(costate));
costate.base_dir = ;
costate.refresh_cache = 1;
- if (checkout_entry(ce, costate, NULL) || lstat(ce-name, st))
+ if (checkout_entry(ce, costate, NULL) || fast_lstat(ce-name, st))
return error(_(cannot checkout %s), ce-name);
return 0;
}
@@ -3253,7 +3253,7 @@ static int load_current(struct image *image, struct patch
*patch)
if (pos 0)
return error(_(%s: does not exist in index), name);
ce = active_cache[pos];
- if (lstat(name, st)) {
+ if (fast_lstat(name, st)) {
if (errno != ENOENT)
return error(_(%s: %s), name, strerror(errno));
if (checkout_target(ce, st))
@@ -3396,7 +3396,7 @@ static int check_preimage(struct patch *patch, struct
cache_entry **ce, struct s
if (previous) {
st_mode = previous-new_mode;
} else if (!cached) {
- stat_ret = lstat(old_name, st);
+ stat_ret = fast_lstat(old_name, st);
if (stat_ret errno != ENOENT)
return error(_(%s: %s), old_name, strerror(errno));
}
@@ -3850,7 +3850,7 @@ static void add_index_file(const char *path, unsigned
mode, void *buf, unsigned
die(_(corrupt patch for subproject %s), path);
} else {
if (!cached) {
- if (lstat(path, st) 0)
+ if (fast_lstat(path, st) 0)
die_errno(_(unable to stat newly created file
'%s'),
path);
fill_stat_cache_info(ce, st);
diff --git a/builtin/commit.c b/builtin/commit.c
index 6b693c1..1d208c6 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -231,7 +231,7 @@ static void add_remove_files(struct string_list *list)
if (p-util)
continue;
- if (!lstat(p-string, st)) {
+ if (!fast_lstat(p-string, st)) {
if (add_to_cache(p-string, st, 0))
die(_(updating files failed));
} else
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 3a410c3..a066719 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -247,7 +247,7 @@ static void show_files(struct dir_struct *dir)
continue;
if (ce_skip_worktree(ce))
continue;
- err = lstat(ce-name, st);
+ err = fast_lstat(ce-name, st);
if (show_deleted err)
show_ce_entry(tag_removed, ce);
if (show_modified ce_modified(ce, st, 0))
diff --git a/builtin/rm.c b/builtin/rm.c
index 06025a2..4b783e7 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -143,7 +143,7 @@ static int check_local_mod(unsigned char *head, int
index_only)
}
ce = active_cache[pos];
- if (lstat(ce-name, st) 0) {
+ if (fast_lstat(ce-name, st)
