Re: svn commit: r1580914 - /subversion/trunk/subversion/libsvn_subr/io.c

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
Received on 2014-03-24 18:39:34 CET