From 545a8b0702d8054b669438e9ad3900c1b11a5123 Mon Sep 17 00:00:00 2001
From: "markus@chromium.org"
<markus@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Thu, 24 Sep 2009 23:53:51 +0000
Subject: [PATCH] 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
Review URL: http://codereview.chromium.org/196053
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@27146 0039d316-1c4b-4281-b951-d872f2087c98
---
base/time.cc | 5 +++
base/time.h | 1 +
base/timer.cc | 2 +-
.../client_socket_pool_base_unittest.cc | 39 +++++++++++++------
net/socket/socket_test_util.cc | 9 +++++
webkit/glue/webkitclient_impl.cc | 16 +++++++-
6 files changed, 58 insertions(+), 14 deletions(-)
diff --git a/base/time.cc b/base/time.cc
index afe8c8f32532d..2c40d21e4bd3b 100644
--- a/base/time.cc
+++ b/base/time.cc
@@ -41,6 +41,11 @@ int64 TimeDelta::InMilliseconds() const {
return delta_ / Time::kMicrosecondsPerMillisecond;
}
+int64 TimeDelta::InMillisecondsRoundedUp() const {
+ return (delta_ + Time::kMicrosecondsPerMillisecond - 1) /
+ Time::kMicrosecondsPerMillisecond;
+}
+
int64 TimeDelta::InMicroseconds() const {
return delta_;
}
diff --git a/base/time.h b/base/time.h
index 6a181afb51415..eb990eff8e764 100644
--- a/base/time.h
+++ b/base/time.h
@@ -71,6 +71,7 @@ class TimeDelta {
int64 InSeconds() const;
double InMillisecondsF() const;
int64 InMilliseconds() const;
+ int64 InMillisecondsRoundedUp() const;
int64 InMicroseconds() const;
TimeDelta& operator=(TimeDelta other) {
diff --git a/base/timer.cc b/base/timer.cc
index 280760681dc2b..ce5fab680efd1 100644
--- a/base/timer.cc
+++ b/base/timer.cc
@@ -22,7 +22,7 @@ void BaseTimer_Helper::InitiateDelayedTask(TimerTask* timer_task) {
delayed_task_->timer_ = this;
MessageLoop::current()->PostDelayedTask(
FROM_HERE, timer_task,
- static_cast<int>(timer_task->delay_.InMilliseconds()));
+ static_cast<int>(timer_task->delay_.InMillisecondsRoundedUp()));
}
} // namespace base
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index 249d7e3dafe9f..c0a04aa277ba1 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -143,31 +143,34 @@ class TestConnectJob : public ConnectJob {
return DoConnect(false /* error */, false /* sync */);
case kMockPendingJob:
set_load_state(LOAD_STATE_CONNECTING);
- MessageLoop::current()->PostTask(
+ MessageLoop::current()->PostDelayedTask(
FROM_HERE,
method_factory_.NewRunnableMethod(
- &TestConnectJob::DoConnect,
- true /* successful */,
- true /* async */));
+ &TestConnectJob::DoConnect,
+ true /* successful */,
+ true /* async */),
+ 2);
return ERR_IO_PENDING;
case kMockPendingFailingJob:
set_load_state(LOAD_STATE_CONNECTING);
- MessageLoop::current()->PostTask(
+ MessageLoop::current()->PostDelayedTask(
FROM_HERE,
method_factory_.NewRunnableMethod(
- &TestConnectJob::DoConnect,
- false /* error */,
- true /* async */));
+ &TestConnectJob::DoConnect,
+ false /* error */,
+ true /* async */),
+ 2);
return ERR_IO_PENDING;
case kMockWaitingJob:
client_socket_factory_->WaitForSignal(this);
waiting_success_ = true;
return ERR_IO_PENDING;
case kMockAdvancingLoadStateJob:
- MessageLoop::current()->PostTask(
+ MessageLoop::current()->PostDelayedTask(
FROM_HERE,
method_factory_.NewRunnableMethod(
- &TestConnectJob::AdvanceLoadState, load_state_));
+ &TestConnectJob::AdvanceLoadState, load_state_),
+ 2);
return ERR_IO_PENDING;
default:
NOTREACHED();
@@ -717,6 +720,8 @@ TEST_F(ClientSocketPoolBaseTest, TotalLimitCountsConnectingSockets) {
// Create one asynchronous request.
connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob);
EXPECT_EQ(ERR_IO_PENDING, StartRequest("d", kDefaultPriority));
+ PlatformThread::Sleep(10);
+ MessageLoop::current()->RunAllPending();
// The next synchronous request should wait for its turn.
connect_job_factory_->set_job_type(TestConnectJob::kMockJob);
@@ -953,14 +958,26 @@ class RequestSocketCallback : public CallbackRunner< Tuple1<int> > {
test_connect_job_factory_->set_job_type(next_job_type_);
handle_->Reset();
within_callback_ = true;
+ TestCompletionCallback next_job_callback;
int rv = InitHandle(
- handle_, "a", kDefaultPriority, this, pool_.get(), NULL);
+ handle_, "a", kDefaultPriority, &next_job_callback, pool_.get(),
+ NULL);
switch (next_job_type_) {
case TestConnectJob::kMockJob:
EXPECT_EQ(OK, rv);
break;
case TestConnectJob::kMockPendingJob:
EXPECT_EQ(ERR_IO_PENDING, rv);
+
+ // For pending jobs, wait for new socket to be created. This makes
+ // sure there are no more pending operations nor any unclosed sockets
+ // when the test finishes.
+ // We need to give it a little bit of time to run, so that all the
+ // operations that happen on timers (e.g. cleanup of idle
+ // connections) can execute.
+ MessageLoop::current()->SetNestableTasksAllowed(true);
+ PlatformThread::Sleep(10);
+ EXPECT_EQ(OK, next_job_callback.WaitForResult());
break;
default:
FAIL() << "Unexpected job type: " << next_job_type_;
diff --git a/net/socket/socket_test_util.cc b/net/socket/socket_test_util.cc
index cd4423be7cc43..7f4af08f892e1 100644
--- a/net/socket/socket_test_util.cc
+++ b/net/socket/socket_test_util.cc
@@ -343,6 +343,15 @@ void ClientSocketPoolTest::SetUp() {
void ClientSocketPoolTest::TearDown() {
// The tests often call Reset() on handles at the end which may post
// DoReleaseSocket() tasks.
+ // Pending tasks created by client_socket_pool_base_unittest.cc are
+ // posted two milliseconds into the future and thus won't become
+ // scheduled until that time.
+ // We wait a few milliseconds to make sure that all such future tasks
+ // are ready to run, before calling RunAllPending(). This will work
+ // correctly even if Sleep() finishes late (and it should never finish
+ // early), as all we have to ensure is that actual wall-time has progressed
+ // past the scheduled starting time of the pending task.
+ PlatformThread::Sleep(10);
MessageLoop::current()->RunAllPending();
}
diff --git a/webkit/glue/webkitclient_impl.cc b/webkit/glue/webkitclient_impl.cc
index 4315db73c381f..79ddffd6d58cd 100644
--- a/webkit/glue/webkitclient_impl.cc
+++ b/webkit/glue/webkitclient_impl.cc
@@ -2,6 +2,7 @@
// source code is governed by a BSD-style license that can be found in the
// LICENSE file.
+#include <math.h>
#include "config.h"
#include "FrameView.h"
@@ -265,12 +266,23 @@ void WebKitClientImpl::setSharedTimerFiredFunction(void (*func)()) {
}
void WebKitClientImpl::setSharedTimerFireTime(double fire_time) {
- int interval = static_cast<int>((fire_time - currentTime()) * 1000);
+ // By converting between double and int64 representation, we run the risk
+ // of losing precision due to rounding errors. Performing computations in
+ // microseconds reduces this risk somewhat. But there still is the potential
+ // of us computing a fire time for the timer that is shorter than what we
+ // need.
+ // As the event loop will check event deadlines prior to actually firing
+ // them, there is a risk of needlessly rescheduling events and of
+ // needlessly looping if sleep times are too short even by small amounts.
+ // This results in measurable performance degradation unless we use ceil() to
+ // always round up the sleep times.
+ int64 interval = static_cast<int64>(
+ ceil((fire_time - currentTime()) * base::Time::kMicrosecondsPerSecond));
if (interval < 0)
interval = 0;
shared_timer_.Stop();
- shared_timer_.Start(base::TimeDelta::FromMilliseconds(interval), this,
+ shared_timer_.Start(base::TimeDelta::FromMicroseconds(interval), this,
&WebKitClientImpl::DoTimeout);
}
--
GitLab