When converting between units of time or data types of different precision,...

When converting between units of time or data types of different precision,
we have to be careful to consistently round in the same direction.

Timeout checks usually check if Now() is less or equal to a deadline in order
to determine if a timeout has occurred. This correctly handles the case where
actual sleep times are equal or longer than requested sleep times.

But if we round down when setting the sleep delay, this can result in
unnecessary and expensive looping. Make sure, we always round up when converting
to a format with less precision.

BUG=none
TEST=none

[Evan Martin, 2009-09-09 22:24:49]

I can't review this properly.  I think Brett or Jim are our best hopes.

[brettw, 2009-09-09 23:03:54]

The change in time is OK, but the call to it and the ceil call are a bit
mysterious. Can you comment why we need to do this in both places?

[jar (doing other things), 2009-09-09 23:55:20]

LGTM (but see nit below)

http://codereview.chromium.org/196053/diff/1/2
File webkit/glue/webkitclient_impl.cc (right):

http://codereview.chromium.org/196053/diff/1/2#newcode225
Line 225: int interval = static_cast<int>(ceil((fire_time - currentTime()) *
1000));
nit: Consider using the manifest constant base::Time::kMillisecondsPerSecond
rather than 1000.

[brettw, 2009-09-11 00:22:12]

In case it's not obvious, be sure to let us know when you upload the new patch.

[Markus (顧孟勤), 2009-09-11 02:06:57]

Oh yes. I'll do that. Got side tracked today w/ a few other issues,
and I don't want to commit it until I am happy w/ the unittests.
Hopefully, tomorrow.


Markus

On Thursday, September 10, 2009,  <brettw@chromium.org> wrote:
> In case it's not obvious, be sure to let us know when you upload the new
patch.
>
> http://codereview.chromium.org/196053
>

[Markus (顧孟勤), 2009-09-22 00:16:38]

Would you mind taking another look. I think, I addressed the (minor) changes
that you asked for.

And I fixed the unittest, which turned out to be somewhat tedious to get right.
The test made all sorts of assumptions about things getting executed right away,
even if they were schedule to happen in the future. The try bots seem to think
that this is correct, now.

This probably caused occasionally random failures in the past, if the try bots
were particularly heavily loaded.

[Markus (顧孟勤), 2009-09-23 16:16:40]

Ping?

On Mon, Sep 21, 2009 at 17:16, <markus@chromium.org> wrote:

> Would you mind taking another look. I think, I addressed the (minor)
> changes
> that you asked for.
>
> And I fixed the unittest, which turned out to be somewhat tedious to get
> right.
> The test made all sorts of assumptions about things getting executed right
> away,
> even if they were schedule to happen in the future. The try bots seem to
> think
> that this is correct, now.
>
> This probably caused occasionally random failures in the past, if the try
> bots
> were particularly heavily loaded.
>
> http://codereview.chromium.org/196053
>

[brettw, 2009-09-23 16:45:16]

LGTM with one style nit.

http://codereview.chromium.org/196053/diff/6001/7001
File webkit/glue/webkitclient_impl.cc (right):

http://codereview.chromium.org/196053/diff/6001/7001#newcode280
Line 280: base::Time::kMicrosecondsPerSecond));
We usually try to avoid random wrapping like this. I'd just put the ceil( on the
next line indented 4 spaces, I think everything will fit inside it on that line
and it will be easier to read.

[jar (doing other things), 2009-09-23 17:23:38]

http://codereview.chromium.org/196053/diff/6001/7005
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/196053/diff/6001/7005#newcode152
Line 152: 2);
style nit: align indentation of arguments.

(same below)

http://codereview.chromium.org/196053/diff/6001/7005#newcode1575
Line 1575: //  MessageLoop::current()->RunAllPending();  // Run the
DoReleaseSocket()
Did you mean to comment this out?  ... or to delete it??

http://codereview.chromium.org/196053/diff/6001/7006
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/196053/diff/6001/7006#newcode346
Line 346: PlatformThread::Sleep(10);
Can you motivate this 10ms sleep?  If you're trying to get this to pass by
adding the sleep, it will surely be flaky :-/.

[Markus (顧孟勤), 2009-09-23 18:12:48]

On Wed, Sep 23, 2009 at 10:23, <jar@chromium.org> wrote:
>
> http://codereview.chromium.org/196053/diff/6001/7005
> File net/socket/client_socket_pool_base_unittest.cc (right):
>
> http://codereview.chromium.org/196053/diff/6001/7005#newcode152
> Line 152: 2);
> style nit: align indentation of arguments.
> (same below)
>

I must be missing something. I don't see how you would prefer it to be
indented. Aren't the arguments already aligned? I am sure I am overlooking
something obvious but would you mind showing me what you would prefer?


> http://codereview.chromium.org/196053/diff/6001/7005#newcode1575
> Line 1575: //  MessageLoop::current()->RunAllPending();  // Run the
> DoReleaseSocket()
> Did you mean to comment this out?  ... or to delete it??
>

Ah. Good catch. Thank you. That was left from debugging the code.


> http://codereview.chromium.org/196053/diff/6001/7006
> File net/socket/socket_test_util.cc (right):
>
> http://codereview.chromium.org/196053/diff/6001/7006#newcode346
> Line 346: PlatformThread::Sleep(10);
> Can you motivate this 10ms sleep?  If you're trying to get this to pass
> by adding the sleep, it will surely be flaky :-/.


I added some comments explaining why we need the sleep.

Previously, the unittest posted the task with a 0 ms delay, assuming that it
would execute the next time pending tasks get executed. But in reality the
actual time when this task would get scheduled is not guaranteed. It could
happen in the same call to the message loop or in the next one.

Despite running things off timers and callbacks, the unittest requires
deterministic behavior to perform its tests. And while I believe the code to
always have been a little unreliable, my change that rounds up sleeping
times, now pretty much guarantees that things would no longer work as
expected by the unittest.

So, I changed the code in the unit test to always explicitly post delayed
tasks two milliseconds into the future (as opposed to the poorly defined
zero milliseconds). This delay will be rounded up and we should see behavior
again, where tasks won't randomly get scheduled on either the first or the
second call to the message loop. But since we now know that delayed tasks
always wait until the second call into the message loop we now need to
occasionally wait for the timeout to actually happen, if we want to force
tasks to get scheduled.

In non-unittest scenarios, this all works automatically. But in the more
artificial unittest environment, we need to add these silly sleeps (see
newly added comment in the source).

You are correct though, that technically the two millisecond delay when
posting a new delayed task is potentially unreliable. On a very slow
machine, two milliseconds might have passed between the call to
PostDelayedTask() and the subsequent entry into the message loop, which
could result in a delayed task getting executed in the first call to the
message loop. I don't think this normally matters for most code paths in the
test, as we normally just call WaitForResult() which is immune to this
problem. But ideally the unittest really needs more deterministic means of
determining when delayed tasks get scheduled.

The only way I can see this happen is by allowing the unittest to control
the advancement of time. So, instead of calling things like Time::Now(), we
should call a method that can be overridden by the unittest. But that would
require some major refactoring of the code, I think.


Markus

[brettw, 2009-09-23 18:19:29]

On Wed, Sep 23, 2009 at 11:12 AM, Markus Gutschke <markus@chromium.org> wrote:
> On Wed, Sep 23, 2009 at 10:23, <jar@chromium.org> wrote:
>>
>> http://codereview.chromium.org/196053/diff/6001/7005
>> File net/socket/client_socket_pool_base_unittest.cc (right):
>>
>> http://codereview.chromium.org/196053/diff/6001/7005#newcode152
>> Line 152: 2);
>> style nit: align indentation of arguments.
>> (same below)
>
> I must be missing something. I don't see how you would prefer it to be
> indented. Aren't the arguments already aligned? I am sure I am overlooking
> something obvious but would you mind showing me what you would prefer?

You did 3 space indent from the NewRunnableMethod, this should
probably be 4, which Jar was probably complaining about.

Brett

[jar (doing other things), 2009-09-23 20:50:52]

http://codereview.chromium.org/196053/diff/6001/7005
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/196053/diff/6001/7005#newcode152
Line 152: 2);
My mistake.  I mismatched the "2" because I didn't see the close paren after the
comment.  The indentation is correct.

...but...

Given that I goofed in my reading, perhaps use of a // style comment beyond the
end of the line would be more readable than the embedded /* ... */ style
comment.  The adjacent // style comments could even be aligned to avoid
interfering with the code flow.  The style guide specifically offers such an
example for describing arguments at a call site.



On 2009/09/23 17:23:38, jar wrote:
> style nit: align indentation of arguments.
> 
> (same below)