diff options
| author | kinuko <kinuko@chromium.org> | 2015-05-05 10:13:51 -0700 |
|---|---|---|
| committer | Commit bot <commit-bot@chromium.org> | 2015-05-05 17:15:18 +0000 |
| commit | f1f70cb5ebe24d85c575396f026a415ed0fe9afc (patch) | |
| tree | 7707d3dbe7c3404c5391318bb015f990e4692d1e | |
| parent | 8d20ad724b073dbb1e2b7e55f3174bdfc9ca2385 (diff) | |
| download | chromium_src-f1f70cb5ebe24d85c575396f026a415ed0fe9afc.zip chromium_src-f1f70cb5ebe24d85c575396f026a415ed0fe9afc.tar.gz chromium_src-f1f70cb5ebe24d85c575396f026a415ed0fe9afc.tar.bz2 | |
Lazily initialize MessageLoop for faster thread startup
Summary of the change and background discussion:
https://docs.google.com/a/chromium.org/document/d/1o1vUUOjX3tC7pV5-nxchaGtElo4NwtzKOAb4Zm09ezw/edit#
This implements approach 1 in the doc.
Approach 2: https://codereview.chromium.org/1086663002/
Approach 3: https://codereview.chromium.org/1058603004/
Discussion thread:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/2t6lB8hUgYw
BUG=465458
Review URL: https://codereview.chromium.org/1011683002
Cr-Commit-Position: refs/heads/master@{#328347}
22 files changed, 265 insertions, 177 deletions
diff --git a/base/message_loop/incoming_task_queue.cc b/base/message_loop/incoming_task_queue.cc index c1ce939..5e9a461 100644 --- a/base/message_loop/incoming_task_queue.cc +++ b/base/message_loop/incoming_task_queue.cc @@ -44,7 +44,8 @@ IncomingTaskQueue::IncomingTaskQueue(MessageLoop* message_loop) message_loop_(message_loop), next_sequence_num_(0), message_loop_scheduled_(false), - always_schedule_work_(AlwaysNotifyPump(message_loop_->type())) { + always_schedule_work_(AlwaysNotifyPump(message_loop_->type())), + is_ready_for_scheduling_(false) { } bool IncomingTaskQueue::AddToIncomingQueue( @@ -109,6 +110,15 @@ void IncomingTaskQueue::WillDestroyCurrentMessageLoop() { message_loop_ = NULL; } +void IncomingTaskQueue::StartScheduling() { + AutoLock lock(incoming_queue_lock_); + DCHECK(!is_ready_for_scheduling_); + DCHECK(!message_loop_scheduled_); + is_ready_for_scheduling_ = true; + if (!incoming_queue_.empty()) + ScheduleWork(); +} + IncomingTaskQueue::~IncomingTaskQueue() { // Verify that WillDestroyCurrentMessageLoop() has been called. DCHECK(!message_loop_); @@ -148,19 +158,25 @@ bool IncomingTaskQueue::PostPendingTask(PendingTask* pending_task) { incoming_queue_.push(*pending_task); pending_task->task.Reset(); - if (always_schedule_work_ || (!message_loop_scheduled_ && was_empty)) { - // Wake up the message loop. - message_loop_->ScheduleWork(); - // After we've scheduled the message loop, we do not need to do so again - // until we know it has processed all of the work in our queue and is - // waiting for more work again. The message loop will always attempt to - // reload from the incoming queue before waiting again so we clear this flag - // in ReloadWorkQueue(). - message_loop_scheduled_ = true; + if (is_ready_for_scheduling_ && + (always_schedule_work_ || (!message_loop_scheduled_ && was_empty))) { + ScheduleWork(); } return true; } +void IncomingTaskQueue::ScheduleWork() { + DCHECK(is_ready_for_scheduling_); + // Wake up the message loop. + message_loop_->ScheduleWork(); + // After we've scheduled the message loop, we do not need to do so again + // until we know it has processed all of the work in our queue and is + // waiting for more work again. The message loop will always attempt to + // reload from the incoming queue before waiting again so we clear this flag + // in ReloadWorkQueue(). + message_loop_scheduled_ = true; +} + } // namespace internal } // namespace base diff --git a/base/message_loop/incoming_task_queue.h b/base/message_loop/incoming_task_queue.h index 72e1f30..7dd1e82 100644 --- a/base/message_loop/incoming_task_queue.h +++ b/base/message_loop/incoming_task_queue.h @@ -53,6 +53,10 @@ class BASE_EXPORT IncomingTaskQueue // Disconnects |this| from the parent message loop. void WillDestroyCurrentMessageLoop(); + // This should be called when the message loop becomes ready for + // scheduling work. + void StartScheduling(); + private: friend class RefCountedThreadSafe<IncomingTaskQueue>; virtual ~IncomingTaskQueue(); @@ -66,6 +70,9 @@ class BASE_EXPORT IncomingTaskQueue // does not retain |pending_task->task| beyond this function call. bool PostPendingTask(PendingTask* pending_task); + // Wakes up the message loop and schedules work. + void ScheduleWork(); + // Number of tasks that require high resolution timing. This value is kept // so that ReloadWorkQueue() completes in constant time. int high_res_task_count_; @@ -92,6 +99,9 @@ class BASE_EXPORT IncomingTaskQueue // if the incoming queue was not empty. const bool always_schedule_work_; + // False until StartScheduling() is called. + bool is_ready_for_scheduling_; + DISALLOW_COPY_AND_ASSIGN(IncomingTaskQueue); }; diff --git a/base/message_loop/message_loop.cc b/base/message_loop/message_loop.cc index eb0f968..4222c77 100644 --- a/base/message_loop/message_loop.cc +++ b/base/message_loop/message_loop.cc @@ -100,6 +100,10 @@ MessagePumpForIO* ToPumpIO(MessagePump* pump) { } #endif // !defined(OS_NACL_SFI) +scoped_ptr<MessagePump> ReturnPump(scoped_ptr<MessagePump> pump) { + return pump; +} + } // namespace //------------------------------------------------------------------------------ @@ -116,41 +120,19 @@ MessageLoop::DestructionObserver::~DestructionObserver() { //------------------------------------------------------------------------------ MessageLoop::MessageLoop(Type type) - : type_(type), -#if defined(OS_WIN) - pending_high_res_tasks_(0), - in_high_res_mode_(false), -#endif - nestable_tasks_allowed_(true), -#if defined(OS_WIN) - os_modal_loop_(false), -#endif // OS_WIN - message_histogram_(NULL), - run_loop_(NULL) { - Init(); - - pump_ = CreateMessagePumpForType(type).Pass(); + : MessageLoop(type, MessagePumpFactoryCallback()) { + BindToCurrentThread(); } MessageLoop::MessageLoop(scoped_ptr<MessagePump> pump) - : pump_(pump.Pass()), - type_(TYPE_CUSTOM), -#if defined(OS_WIN) - pending_high_res_tasks_(0), - in_high_res_mode_(false), -#endif - nestable_tasks_allowed_(true), -#if defined(OS_WIN) - os_modal_loop_(false), -#endif // OS_WIN - message_histogram_(NULL), - run_loop_(NULL) { - DCHECK(pump_.get()); - Init(); + : MessageLoop(TYPE_CUSTOM, Bind(&ReturnPump, Passed(&pump))) { + BindToCurrentThread(); } MessageLoop::~MessageLoop() { - DCHECK_EQ(this, current()); + // current() could be NULL if this message loop is destructed before it is + // bound to a thread. + DCHECK(current() == this || !current()); // iOS just attaches to the loop, it doesn't Run it. // TODO(stuartmorgan): Consider wiring up a Detach(). @@ -299,11 +281,13 @@ void MessageLoop::PostNonNestableDelayedTask( } void MessageLoop::Run() { + DCHECK(pump_); RunLoop run_loop; run_loop.Run(); } void MessageLoop::RunUntilIdle() { + DCHECK(pump_); RunLoop run_loop; run_loop.RunUntilIdle(); } @@ -383,13 +367,43 @@ bool MessageLoop::IsIdleForTesting() { //------------------------------------------------------------------------------ -void MessageLoop::Init() { +scoped_ptr<MessageLoop> MessageLoop::CreateUnbound( + Type type, MessagePumpFactoryCallback pump_factory) { + return make_scoped_ptr(new MessageLoop(type, pump_factory)); +} + +MessageLoop::MessageLoop(Type type, MessagePumpFactoryCallback pump_factory) + : type_(type), +#if defined(OS_WIN) + pending_high_res_tasks_(0), + in_high_res_mode_(false), +#endif + nestable_tasks_allowed_(true), +#if defined(OS_WIN) + os_modal_loop_(false), +#endif // OS_WIN + pump_factory_(pump_factory), + message_histogram_(NULL), + run_loop_(NULL), + incoming_task_queue_(new internal::IncomingTaskQueue(this)), + message_loop_proxy_( + new internal::MessageLoopProxyImpl(incoming_task_queue_)) { + // If type is TYPE_CUSTOM non-null pump_factory must be given. + DCHECK_EQ(type_ == TYPE_CUSTOM, !pump_factory_.is_null()); +} + +void MessageLoop::BindToCurrentThread() { + DCHECK(!pump_); + if (!pump_factory_.is_null()) + pump_ = pump_factory_.Run(); + else + pump_ = CreateMessagePumpForType(type_); + DCHECK(!current()) << "should only have one message loop per thread"; lazy_tls_ptr.Pointer()->Set(this); - incoming_task_queue_ = new internal::IncomingTaskQueue(this); - message_loop_proxy_ = - new internal::MessageLoopProxyImpl(incoming_task_queue_); + incoming_task_queue_->StartScheduling(); + message_loop_proxy_->BindToCurrentThread(); thread_task_runner_handle_.reset( new ThreadTaskRunnerHandle(message_loop_proxy_)); } diff --git a/base/message_loop/message_loop.h b/base/message_loop/message_loop.h index fd7596a..f2f89d0 100644 --- a/base/message_loop/message_loop.h +++ b/base/message_loop/message_loop.h @@ -114,7 +114,8 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate { explicit MessageLoop(Type type = TYPE_DEFAULT); // Creates a TYPE_CUSTOM MessageLoop with the supplied MessagePump, which must // be non-NULL. - explicit MessageLoop(scoped_ptr<base::MessagePump> pump); + explicit MessageLoop(scoped_ptr<MessagePump> pump); + ~MessageLoop() override; // Returns the MessageLoop object for the current thread, or null if none. @@ -394,10 +395,6 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate { // Returns true if the message loop is "idle". Provided for testing. bool IsIdleForTesting(); - // Wakes up the message pump. Can be called on any thread. The caller is - // responsible for synchronizing ScheduleWork() calls. - void ScheduleWork(); - // Returns the TaskAnnotator which is used to add debug information to posted // tasks. debug::TaskAnnotator* task_annotator() { return &task_annotator_; } @@ -411,9 +408,33 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate { private: friend class RunLoop; + friend class internal::IncomingTaskQueue; + friend class ScheduleWorkTest; + friend class Thread; + + using MessagePumpFactoryCallback = Callback<scoped_ptr<MessagePump>()>; - // Configures various members for the two constructors. - void Init(); + // Creates a MessageLoop without binding to a thread. + // If |type| is TYPE_CUSTOM non-null |pump_factory| must be also given + // to create a message pump for this message loop. Otherwise a default + // message pump for the |type| is created. + // + // It is valid to call this to create a new message loop on one thread, + // and then pass it to the thread where the message loop actually runs. + // The message loop's BindToCurrentThread() method must be called on the + // thread the message loop runs on, before calling Run(). + // Before BindToCurrentThread() is called only Post*Task() functions can + // be called on the message loop. + scoped_ptr<MessageLoop> CreateUnbound( + Type type, + MessagePumpFactoryCallback pump_factory); + + // Common private constructor. Other constructors delegate the initialization + // to this constructor. + MessageLoop(Type type, MessagePumpFactoryCallback pump_factory); + + // Configure various members and bind this message loop to the current thread. + void BindToCurrentThread(); // Invokes the actual run loop using the message pump. void RunHandler(); @@ -437,6 +458,10 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate { // empty. void ReloadWorkQueue(); + // Wakes up the message pump. Can be called on any thread. The caller is + // responsible for synchronizing ScheduleWork() calls. + void ScheduleWork(); + // Start recording histogram info about events and action IF it was enabled // and IF the statistics recorder can accept a registration of our histogram. void StartHistogrammer(); @@ -490,6 +515,10 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate { bool os_modal_loop_; #endif + // pump_factory_.Run() is called to create a message pump for this loop + // if type_ is TYPE_CUSTOM and pump_ is null. + MessagePumpFactoryCallback pump_factory_; + std::string thread_name_; // A profiling histogram showing the counts of various messages and events. HistogramBase* message_histogram_; diff --git a/base/message_loop/message_loop_proxy_impl.cc b/base/message_loop/message_loop_proxy_impl.cc index b7abca3..0da21a0 100644 --- a/base/message_loop/message_loop_proxy_impl.cc +++ b/base/message_loop/message_loop_proxy_impl.cc @@ -15,7 +15,12 @@ namespace internal { MessageLoopProxyImpl::MessageLoopProxyImpl( scoped_refptr<IncomingTaskQueue> incoming_queue) : incoming_queue_(incoming_queue), - valid_thread_id_(PlatformThread::CurrentId()) { + valid_thread_id_(kInvalidThreadId) { +} + +void MessageLoopProxyImpl::BindToCurrentThread() { + DCHECK_EQ(kInvalidThreadId, valid_thread_id_); + valid_thread_id_ = PlatformThread::CurrentId(); } bool MessageLoopProxyImpl::PostDelayedTask( diff --git a/base/message_loop/message_loop_proxy_impl.h b/base/message_loop/message_loop_proxy_impl.h index 0fe629f..c477320 100644 --- a/base/message_loop/message_loop_proxy_impl.h +++ b/base/message_loop/message_loop_proxy_impl.h @@ -24,6 +24,9 @@ class BASE_EXPORT MessageLoopProxyImpl : public MessageLoopProxy { explicit MessageLoopProxyImpl( scoped_refptr<IncomingTaskQueue> incoming_queue); + // Initialize this message loop proxy on the current thread. + void BindToCurrentThread(); + // MessageLoopProxy implementation bool PostDelayedTask(const tracked_objects::Location& from_here, const base::Closure& task, diff --git a/base/message_loop/message_pump_perftest.cc b/base/message_loop/message_pump_perftest.cc index b3e5604..0bf3d0c 100644 --- a/base/message_loop/message_pump_perftest.cc +++ b/base/message_loop/message_pump_perftest.cc @@ -20,7 +20,6 @@ #endif namespace base { -namespace { class ScheduleWorkTest : public testing::Test { public: @@ -224,9 +223,6 @@ TEST_F(ScheduleWorkTest, ThreadTimeToJavaFromFourThreads) { } #endif -static void DoNothing() { -} - class FakeMessagePump : public MessagePump { public: FakeMessagePump() {} @@ -289,5 +285,4 @@ TEST_F(PostTaskTest, OneHundredTasksPerReload) { Run(1000, 100); } -} // namespace } // namespace base diff --git a/base/threading/platform_thread.h b/base/threading/platform_thread.h index d8f06e5..69a2b0d 100644 --- a/base/threading/platform_thread.h +++ b/base/threading/platform_thread.h @@ -89,6 +89,10 @@ class PlatformThreadHandle { id_(id) { } + PlatformThreadId id() const { + return id_; + } + bool is_equal(const PlatformThreadHandle& other) const { return handle_ == other.handle_; } diff --git a/base/threading/platform_thread_win.cc b/base/threading/platform_thread_win.cc index 4eb2cb2..aeaa7c7 100644 --- a/base/threading/platform_thread_win.cc +++ b/base/threading/platform_thread_win.cc @@ -108,15 +108,16 @@ bool CreateThreadInternal(size_t stack_size, // have to work running on CreateThread() threads anyway, since we run code // on the Windows thread pool, etc. For some background on the difference: // http://www.microsoft.com/msj/1099/win32/win321099.aspx + PlatformThreadId thread_id; void* thread_handle = CreateThread( - NULL, stack_size, ThreadFunc, params, flags, NULL); + NULL, stack_size, ThreadFunc, params, flags, &thread_id); if (!thread_handle) { delete params; return false; } if (out_thread_handle) - *out_thread_handle = PlatformThreadHandle(thread_handle); + *out_thread_handle = PlatformThreadHandle(thread_handle, thread_id); else CloseHandle(thread_handle); return true; diff --git a/base/threading/thread.cc b/base/threading/thread.cc index 0e4aab2..bd565c9 100644 --- a/base/threading/thread.cc +++ b/base/threading/thread.cc @@ -37,20 +37,6 @@ void ThreadQuitHelper() { Thread::SetThreadWasQuitProperly(true); } -// Used to pass data to ThreadMain. This structure is allocated on the stack -// from within StartWithOptions. -struct Thread::StartupData { - // We get away with a const reference here because of how we are allocated. - const Thread::Options& options; - - // Used to synchronize thread startup. - WaitableEvent event; - - explicit StartupData(const Options& opt) - : options(opt), - event(false, false) {} -}; - Thread::Options::Options() : message_loop_type(MessageLoop::TYPE_DEFAULT), timer_slack(TIMER_SLACK_NONE), @@ -72,13 +58,11 @@ Thread::Thread(const std::string& name) #if defined(OS_WIN) com_status_(NONE), #endif - started_(false), stopping_(false), running_(false), - startup_data_(NULL), thread_(0), - message_loop_(NULL), - thread_id_(kInvalidThreadId), + message_loop_(nullptr), + message_loop_timer_slack_(TIMER_SLACK_NONE), name_(name) { } @@ -104,34 +88,45 @@ bool Thread::StartWithOptions(const Options& options) { SetThreadWasQuitProperly(false); - StartupData startup_data(options); - startup_data_ = &startup_data; + MessageLoop::Type type = options.message_loop_type; + if (!options.message_pump_factory.is_null()) + type = MessageLoop::TYPE_CUSTOM; + + message_loop_timer_slack_ = options.timer_slack; + message_loop_ = new MessageLoop(type, options.message_pump_factory); + + start_event_.reset(new WaitableEvent(false, false)); if (!PlatformThread::Create(options.stack_size, this, &thread_)) { DLOG(ERROR) << "failed to create thread"; - startup_data_ = NULL; + delete message_loop_; + message_loop_ = nullptr; + start_event_.reset(); return false; } - // TODO(kinuko): Remove once crbug.com/465458 is solved. - tracked_objects::ScopedTracker tracking_profile_wait( - FROM_HERE_WITH_EXPLICIT_FUNCTION( - "465458 base::Thread::StartWithOptions (Wait)")); - - // Wait for the thread to start and initialize message_loop_ - base::ThreadRestrictions::ScopedAllowWait allow_wait; - startup_data.event.Wait(); + DCHECK(message_loop_); + return true; +} - // set it to NULL so we don't keep a pointer to some object on the stack. - startup_data_ = NULL; - started_ = true; +bool Thread::StartAndWaitForTesting() { + bool result = Start(); + if (!result) + return false; + WaitUntilThreadStarted(); + return result; +} - DCHECK(message_loop_); +bool Thread::WaitUntilThreadStarted() { + if (!start_event_) + return false; + base::ThreadRestrictions::ScopedAllowWait allow_wait; + start_event_->Wait(); return true; } void Thread::Stop() { - if (!started_) + if (!start_event_) return; StopSoon(); @@ -147,7 +142,7 @@ void Thread::Stop() { DCHECK(!message_loop_); // The thread no longer needs to be joined. - started_ = false; + start_event_.reset(); stopping_ = false; } @@ -155,9 +150,7 @@ void Thread::Stop() { void Thread::StopSoon() { // We should only be called on the same thread that started us. - // Reading thread_id_ without a lock can lead to a benign data race - // with ThreadMain, so we annotate it to stay silent under ThreadSanitizer. - DCHECK_NE(ANNOTATE_UNPROTECTED_READ(thread_id_), PlatformThread::CurrentId()); + DCHECK_NE(thread_id(), PlatformThread::CurrentId()); if (stopping_ || !message_loop_) return; @@ -167,13 +160,22 @@ void Thread::StopSoon() { } bool Thread::IsRunning() const { + // If the thread's already started (i.e. message_loop_ is non-null) and + // not yet requested to stop (i.e. stopping_ is false) we can just return + // true. (Note that stopping_ is touched only on the same thread that + // starts / started the new thread so we need no locking here.) + if (message_loop_ && !stopping_) + return true; + // Otherwise check the running_ flag, which is set to true by the new thread + // only while it is inside Run(). + AutoLock lock(lock_); return running_; } void Thread::SetPriority(ThreadPriority priority) { // The thread must be started (and id known) for this to be // compatible with all platforms. - DCHECK_NE(thread_id_, kInvalidThreadId); + DCHECK(message_loop_ != nullptr); PlatformThread::SetThreadPriority(thread_, priority); } @@ -194,60 +196,57 @@ bool Thread::GetThreadWasQuitProperly() { } void Thread::ThreadMain() { - { - // The message loop for this thread. - // Allocated on the heap to centralize any leak reports at this line. - scoped_ptr<MessageLoop> message_loop; - if (!startup_data_->options.message_pump_factory.is_null()) { - message_loop.reset( - new MessageLoop(startup_data_->options.message_pump_factory.Run())); - } else { - message_loop.reset( - new MessageLoop(startup_data_->options.message_loop_type)); - } - - // Complete the initialization of our Thread object. - thread_id_ = PlatformThread::CurrentId(); - PlatformThread::SetName(name_); - ANNOTATE_THREAD_NAME(name_.c_str()); // Tell the name to race detector. - message_loop->set_thread_name(name_); - message_loop->SetTimerSlack(startup_data_->options.timer_slack); - message_loop_ = message_loop.get(); + // Complete the initialization of our Thread object. + DCHECK_EQ(thread_id(), PlatformThread::CurrentId()); + PlatformThread::SetName(name_.c_str()); + ANNOTATE_THREAD_NAME(name_.c_str()); // Tell the name to race detector. + + // Lazily initialize the message_loop so that it can run on this thread. + DCHECK(message_loop_); + scoped_ptr<MessageLoop> message_loop(message_loop_); + message_loop_->BindToCurrentThread(); + message_loop_->set_thread_name(name_); + message_loop_->SetTimerSlack(message_loop_timer_slack_); #if defined(OS_WIN) - scoped_ptr<win::ScopedCOMInitializer> com_initializer; - if (com_status_ != NONE) { - com_initializer.reset((com_status_ == STA) ? - new win::ScopedCOMInitializer() : - new win::ScopedCOMInitializer(win::ScopedCOMInitializer::kMTA)); - } + scoped_ptr<win::ScopedCOMInitializer> com_initializer; + if (com_status_ != NONE) { + com_initializer.reset((com_status_ == STA) ? + new win::ScopedCOMInitializer() : + new win::ScopedCOMInitializer(win::ScopedCOMInitializer::kMTA)); + } #endif - // Let the thread do extra initialization. - // Let's do this before signaling we are started. - Init(); + // Let the thread do extra initialization. + Init(); + { + AutoLock lock(lock_); running_ = true; - startup_data_->event.Signal(); - // startup_data_ can't be touched anymore since the starting thread is now - // unlocked. + } + + start_event_->Signal(); + + Run(message_loop_); - Run(message_loop_); + { + AutoLock lock(lock_); running_ = false; + } - // Let the thread do extra cleanup. - CleanUp(); + // Let the thread do extra cleanup. + CleanUp(); #if defined(OS_WIN) - com_initializer.reset(); + com_initializer.reset(); #endif - // Assert that MessageLoop::Quit was called by ThreadQuitHelper. - DCHECK(GetThreadWasQuitProperly()); + // Assert that MessageLoop::Quit was called by ThreadQuitHelper. + DCHECK(GetThreadWasQuitProperly()); - // We can't receive messages anymore. - message_loop_ = NULL; - } + // We can't receive messages anymore. + // (The message loop is destructed at the end of this block) + message_loop_ = NULL; } } // namespace base diff --git a/base/threading/thread.h b/base/threading/thread.h index 4915606..2e19b0a 100644 --- a/base/threading/thread.h +++ b/base/threading/thread.h @@ -13,11 +13,13 @@ #include "base/message_loop/message_loop.h" #include "base/message_loop/timer_slack.h" #include "base/single_thread_task_runner.h" +#include "base/synchronization/lock.h" #include "base/threading/platform_thread.h" namespace base { class MessagePump; +class WaitableEvent; // A simple thread abstraction that establishes a MessageLoop on a new thread. // The consumer uses the MessageLoop of the thread to cause code to execute on @@ -45,7 +47,7 @@ class BASE_EXPORT Thread : PlatformThread::Delegate { // This is ignored if message_pump_factory.is_null() is false. MessageLoop::Type message_loop_type; - // Specify timer slack for thread message loop. + // Specifies timer slack for thread message loop. TimerSlack timer_slack; // Used to create the MessagePump for the MessageLoop. The callback is Run() @@ -81,7 +83,7 @@ class BASE_EXPORT Thread : PlatformThread::Delegate { // init_com_with_mta(false) and then StartWithOptions() with any message loop // type other than TYPE_UI. void init_com_with_mta(bool use_mta) { - DCHECK(!started_); + DCHECK(!start_event_); com_status_ = use_mta ? MTA : STA; } #endif @@ -103,6 +105,18 @@ class BASE_EXPORT Thread : PlatformThread::Delegate { // callback. bool StartWithOptions(const Options& options); + // Starts the thread and wait for the thread to start and run initialization + // before returning. It's same as calling Start() and then + // WaitUntilThreadStarted(). + // Note that using this (instead of Start() or StartWithOptions() causes + // jank on the calling thread, should be used only in testing code. + bool StartAndWaitForTesting(); + + // Blocks until the thread starts running. Called within StartAndWait(). + // Note that calling this causes jank on the calling thread, must be used + // carefully for production code. + bool WaitUntilThreadStarted(); + // Signals the thread to exit and returns once the thread has exited. After // this method returns, the Thread object is completely reset and may be used // as if it were newly constructed (i.e., Start may be called again). @@ -166,7 +180,7 @@ class BASE_EXPORT Thread : PlatformThread::Delegate { PlatformThreadHandle thread_handle() { return thread_; } // The thread ID. - PlatformThreadId thread_id() const { return thread_id_; } + PlatformThreadId thread_id() const { return thread_.id(); } // Returns true if the thread has been started, and not yet stopped. bool IsRunning() const; @@ -208,19 +222,13 @@ class BASE_EXPORT Thread : PlatformThread::Delegate { ComStatus com_status_; #endif - // Whether we successfully started the thread. - bool started_; - // If true, we're in the middle of stopping, and shouldn't access // |message_loop_|. It may non-NULL and invalid. bool stopping_; // True while inside of Run(). bool running_; - - // Used to pass data to ThreadMain. - struct StartupData; - StartupData* startup_data_; + mutable base::Lock lock_; // Protects running_. // The thread's handle. PlatformThreadHandle thread_; @@ -229,12 +237,16 @@ class BASE_EXPORT Thread : PlatformThread::Delegate { // by the created thread. MessageLoop* message_loop_; - // Our thread's ID. - PlatformThreadId thread_id_; + // Stores Options::timer_slack_ until the message loop has been bound to + // a thread. + TimerSlack message_loop_timer_slack_; // The name of the thread. Used for debugging purposes. std::string name_; + // Non-null if the thread has successfully started. + scoped_ptr<WaitableEvent> start_event_; + friend void ThreadQuitHelper(); DISALLOW_COPY_AND_ASSIGN(Thread); diff --git a/base/threading/thread_id_name_manager_unittest.cc b/base/threading/thread_id_name_manager_unittest.cc index b5953d5..b17c681 100644 --- a/base/threading/thread_id_name_manager_unittest.cc +++ b/base/threading/thread_id_name_manager_unittest.cc @@ -21,8 +21,8 @@ TEST_F(ThreadIdNameManagerTest, AddThreads) { base::Thread thread_a(kAThread); base::Thread thread_b(kBThread); - thread_a.Start(); - thread_b.Start(); + thread_a.StartAndWaitForTesting(); + thread_b.StartAndWaitForTesting(); EXPECT_STREQ(kAThread, manager->GetName(thread_a.thread_id())); EXPECT_STREQ(kBThread, manager->GetName(thread_b.thread_id())); @@ -35,10 +35,10 @@ TEST_F(ThreadIdNameManagerTest, RemoveThreads) { base::ThreadIdNameManager* manager = base::ThreadIdNameManager::GetInstance(); base::Thread thread_a(kAThread); - thread_a.Start(); + thread_a.StartAndWaitForTesting(); { base::Thread thread_b(kBThread); - thread_b.Start(); + thread_b.StartAndWaitForTesting(); thread_b.Stop(); } EXPECT_STREQ(kAThread, manager->GetName(thread_a.thread_id())); @@ -51,12 +51,12 @@ TEST_F(ThreadIdNameManagerTest, RestartThread) { base::ThreadIdNameManager* manager = base::ThreadIdNameManager::GetInstance(); base::Thread thread_a(kAThread); - thread_a.Start(); + thread_a.StartAndWaitForTesting(); base::PlatformThreadId a_id = thread_a.thread_id(); EXPECT_STREQ(kAThread, manager->GetName(a_id)); thread_a.Stop(); - thread_a.Start(); + thread_a.StartAndWaitForTesting(); EXPECT_STREQ("", manager->GetName(a_id)); EXPECT_STREQ(kAThread, manager->GetName(thread_a.thread_id())); thread_a.Stop(); diff --git a/base/threading/thread_unittest.cc b/base/threading/thread_unittest.cc index a89768e..e86c758 100644 --- a/base/threading/thread_unittest.cc +++ b/base/threading/thread_unittest.cc @@ -194,11 +194,12 @@ TEST_F(ThreadTest, ThreadName) { EXPECT_EQ("ThreadName", a.thread_name()); } -// Make sure we can't use a thread between Start() and Init(). +// Make sure Init() is called after Start() and before +// WaitUntilThreadInitialized() returns. TEST_F(ThreadTest, SleepInsideInit) { SleepInsideInitThread t; EXPECT_FALSE(t.InitCalled()); - t.Start(); + t.StartAndWaitForTesting(); EXPECT_TRUE(t.InitCalled()); } diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc index 874a9ca..80f3bea 100644 --- a/chrome/browser/io_thread.cc +++ b/chrome/browser/io_thread.cc @@ -619,11 +619,6 @@ net::URLRequestContextGetter* IOThread::system_url_request_context_getter() { } void IOThread::Init() { - // Prefer to use InitAsync unless you need initialization to block - // the UI thread -} - -void IOThread::InitAsync() { // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/466432 // is fixed. tracked_objects::ScopedTracker tracking_profile1( diff --git a/chrome/browser/io_thread.h b/chrome/browser/io_thread.h index 8ca3443..831e62f 100644 --- a/chrome/browser/io_thread.h +++ b/chrome/browser/io_thread.h @@ -254,7 +254,6 @@ class IOThread : public content::BrowserThreadDelegate { // This handles initialization and destruction of state that must // live on the IO thread. void Init() override; - void InitAsync() override; void CleanUp() override; // Initializes |params| based on the settings in |globals|. diff --git a/chrome/browser/metrics/thread_watcher_unittest.cc b/chrome/browser/metrics/thread_watcher_unittest.cc index 4b3c457..00171b8 100644 --- a/chrome/browser/metrics/thread_watcher_unittest.cc +++ b/chrome/browser/metrics/thread_watcher_unittest.cc @@ -257,9 +257,9 @@ class ThreadWatcherTest : public ::testing::Test { db_thread_.reset(new content::TestBrowserThread(BrowserThread::DB)); io_thread_.reset(new content::TestBrowserThread(BrowserThread::IO)); watchdog_thread_.reset(new WatchDogThread()); - db_thread_->Start(); - io_thread_->Start(); - watchdog_thread_->Start(); + db_thread_->StartAndWaitForTesting(); + io_thread_->StartAndWaitForTesting(); + watchdog_thread_->StartAndWaitForTesting(); WatchDogThread::PostTask( FROM_HERE, @@ -691,7 +691,7 @@ TEST_F(ThreadWatcherListTest, Restart) { content::TestBrowserThread ui_thread(BrowserThread::UI, &message_loop_for_ui); scoped_ptr<WatchDogThread> watchdog_thread_(new WatchDogThread()); - watchdog_thread_->Start(); + watchdog_thread_->StartAndWaitForTesting(); // See http://crbug.com/347887. // StartWatchingAll() will PostDelayedTask to create g_thread_watcher_list_, diff --git a/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc b/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc index 3db65e9..34adb95 100644 --- a/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc +++ b/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc @@ -336,6 +336,9 @@ TEST_F(SyncBackendRegistrarShutdownTest, BlockingShutdown) { base::Bind(&SyncBackendRegistrar::Shutdown, base::Unretained(registrar.release()))); + // Make sure the thread starts running. + sync_thread->WaitUntilThreadStarted(); + // The test verifies that the sync thread doesn't block because // of the blocked DB thread and can finish the shutdown. sync_thread->message_loop()->RunUntilIdle(); diff --git a/content/browser/browser_thread_impl.cc b/content/browser/browser_thread_impl.cc index 78e3836..4ae6607 100644 --- a/content/browser/browser_thread_impl.cc +++ b/content/browser/browser_thread_impl.cc @@ -159,14 +159,8 @@ void BrowserThreadImpl::Init() { AtomicWord stored_pointer = base::subtle::NoBarrier_Load(storage); BrowserThreadDelegate* delegate = reinterpret_cast<BrowserThreadDelegate*>(stored_pointer); - if (delegate) { + if (delegate) delegate->Init(); - message_loop()->PostTask(FROM_HERE, - base::Bind(&BrowserThreadDelegate::InitAsync, - // Delegate is expected to exist for the - // duration of the thread's lifetime - base::Unretained(delegate))); - } } // We disable optimizations for this block of functions so the compiler doesn't diff --git a/content/public/browser/browser_thread_delegate.h b/content/public/browser/browser_thread_delegate.h index ad5fd7e..3688e65 100644 --- a/content/public/browser/browser_thread_delegate.h +++ b/content/public/browser/browser_thread_delegate.h @@ -22,9 +22,6 @@ class BrowserThreadDelegate { // Called prior to starting the message loop virtual void Init() = 0; - // Called as the first task on the thread's message loop. - virtual void InitAsync() = 0; - // Called just after the message loop ends. virtual void CleanUp() = 0; }; diff --git a/content/public/test/test_browser_thread.cc b/content/public/test/test_browser_thread.cc index ac67480..62d9eb8 100644 --- a/content/public/test/test_browser_thread.cc +++ b/content/public/test/test_browser_thread.cc @@ -58,6 +58,10 @@ bool TestBrowserThread::Start() { return impl_->Start(); } +bool TestBrowserThread::StartAndWaitForTesting() { + return impl_->StartAndWaitForTesting(); +} + bool TestBrowserThread::StartIOThread() { base::Thread::Options options; options.message_loop_type = base::MessageLoop::TYPE_IO; diff --git a/content/public/test/test_browser_thread.h b/content/public/test/test_browser_thread.h index 2428bbb..d3a2564 100644 --- a/content/public/test/test_browser_thread.h +++ b/content/public/test/test_browser_thread.h @@ -36,6 +36,10 @@ class TestBrowserThread { // Starts the thread with a generic message loop. bool Start(); + // Starts the thread with a generic message loop and waits for the + // thread to run. + bool StartAndWaitForTesting(); + // Starts the thread with an IOThread message loop. bool StartIOThread(); diff --git a/net/android/network_change_notifier_android.cc b/net/android/network_change_notifier_android.cc index 36ebda6..3224c63 100644 --- a/net/android/network_change_notifier_android.cc +++ b/net/android/network_change_notifier_android.cc @@ -172,6 +172,9 @@ NetworkChangeNotifierAndroid::NetworkChangeNotifierAndroid( delegate_->AddObserver(this); dns_config_service_thread_->StartWithOptions( base::Thread::Options(base::MessageLoop::TYPE_IO, 0)); + // Wait until Init is called on the DNS config thread before + // calling InitAfterStart. + dns_config_service_thread_->WaitUntilThreadStarted(); dns_config_service_thread_->InitAfterStart(); } |