You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2014/03/24 18:07:30 UTC
RE: svn commit: r1580914 - /subversion/trunk/subversion/libsvn_subr/io.c
> -----Original Message-----
> From: julianfoad@apache.org [mailto:julianfoad@apache.org]
> Sent: maandag 24 maart 2014 17:45
> To: commits@subversion.apache.org
> Subject: svn commit: r1580914 -
> /subversion/trunk/subversion/libsvn_subr/io.c
>
> Author: julianfoad
> Date: Mon Mar 24 16:45:26 2014
> New Revision: 1580914
>
> URL: http://svn.apache.org/r1580914
> Log:
> * subversion/svn/notify.c
> (svn_io_sleep_for_timestamps): Simplify, eliminating a return path that
> looked
> like a potential source of bugs but was probably in fact safe.
As noted on IRC, this patch can make us wait for the next... next second... by determining to what second we wait later.
I don't see why this patch improves the current behavior of this function.
The if case you removed checked if the time to wait for is in the past, which may happen for various reasons... E.g.g when the stat operation is somehow slow... or because the OS-scheduler paged us out and only returned later.
In these cases we now just wait one second extra...
Originally we always waited (<= 1.5), but then we made use of the delay time to determine if we really had to wait. Doing this with this code made sure the new code was not *more expensive* than the old check.
But now the code is updated to still wait the old long time... after we spend time determining whether we should wait at all.
Bert
Re: svn commit: r1580914 - /subversion/trunk/subversion/libsvn_subr/io.c
Posted by Branko Čibej <br...@wandisco.com>.
On 24.03.2014 18:31, Julian Foad wrote:
> p.s. For other readers, I only started looking at this at all because
> Philip reported an actual bug whereby the function's 1-millisecond
> delay is simply wrong for many Linux systems. - Julian
We really should not rely on the system time resolution but on the
resolution of the filesystem mtime, which can vary from less than a
millisecond to 2 seconds (hello, FAT), and is almost always different
than the system timer resolution.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1580914 - /subversion/trunk/subversion/libsvn_subr/io.c
Posted by Julian Foad <ju...@btopenworld.com>.
I reverted this in r1580926.
Bert Huijben wrote:
>> Log:
>> * subversion/svn/notify.c
>> (svn_io_sleep_for_timestamps): Simplify, eliminating a return path that
>> looked
>> like a potential source of bugs but was probably in fact safe.
>
> As noted on IRC, this patch can make us wait for the next... next second... by
> determining to what second we wait later.
It effectively added one 'stat' to the code path for whatever client-level operation was performed.
> I don't see why this patch improves the current behavior of this function.
It doesn't, it was only to simplify the code and remove what looked like a dodgy return path.
> The if case you removed checked if the time to wait for is in the past, which
> may happen for various reasons... E.g.g when the stat operation is somehow
> slow... or because the OS-scheduler paged us out and only returned later.
As I explained on IRC, my version eliminated the possibility of calculating a negative delay.
> In these cases we now just wait one second extra...
No, not one extra second; we just wait till the next one-second boundary starting from after the 'stat' rather than starting from before the 'stat'. That adds one 'stat' time on average.
> Originally we always waited (<= 1.5), but then we made use of the delay time
> to determine if we really had to wait. Doing this with this code made sure the
> new code was not *more expensive* than the old check.
Agreed. And that was a nice touch.
> But now the code is updated to still wait the old long time... after we spend
> time determining whether we should wait at all.
I've reverted it because I want you to be happy with it and I don't care that much -- the whole thing is a horribly crude hack anyway. If we care about it at all we should make it figure out how long it really needs to wait.
p.s. For other readers, I only started looking at this at all because Philip reported an actual bug whereby the function's 1-millisecond delay is simply wrong for many Linux systems.
- Julian