Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(643)

Unified Diff: content/browser/browser_thread_impl.cc

Issue 2464233002: Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler (Closed)
Patch Set: Rely on Thread::using_external_message_loop_ instead of hard-coding BrowserThread::UI Created 4 years, 1 month ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: content/browser/browser_thread_impl.cc
diff --git a/content/browser/browser_thread_impl.cc b/content/browser/browser_thread_impl.cc
index 18eff7c8672e37ecf83381b566474c4d75fc9af3..33099cd0e1b4920d02e95ad5fec1474576dc7883 100644
--- a/content/browser/browser_thread_impl.cc
+++ b/content/browser/browser_thread_impl.cc
@@ -12,10 +12,12 @@
#include "base/bind.h"
#include "base/compiler_specific.h"
#include "base/lazy_instance.h"
+#include "base/logging.h"
#include "base/macros.h"
+#include "base/message_loop/message_loop.h"
#include "base/profiler/scoped_tracker.h"
#include "base/run_loop.h"
-#include "base/single_thread_task_runner.h"
+#include "base/task_scheduler/task_scheduler.h"
#include "base/threading/platform_thread.h"
#include "base/threading/sequenced_worker_pool.h"
#include "build/build_config.h"
@@ -52,6 +54,7 @@ static const char* GetThreadName(BrowserThread::ID thread) {
// An implementation of SingleThreadTaskRunner to be used in conjunction
// with BrowserThread.
+// TODO(gab): This can go away in favor of |g_globals->task_runners|.
class BrowserThreadTaskRunner : public base::SingleThreadTaskRunner {
public:
explicit BrowserThreadTaskRunner(BrowserThread::ID identifier)
@@ -99,31 +102,45 @@ struct BrowserThreadTaskRunners {
base::LazyInstance<BrowserThreadTaskRunners>::Leaky g_task_runners =
LAZY_INSTANCE_INITIALIZER;
+// State of a given BrowserThread::ID in chronological order throughout the
+// browser process' lifetime.
+enum BrowserThreadState {
+ // BrowserThread::ID isn't associated with anything yet.
+ UNINITIALIZED = 0,
+ // BrowserThread::ID is associated but the underlying thread doesn't run yet.
Avi (use Gerrit) 2016/11/08 16:16:15 "... underlying thread hasn't run yet."
gab 2016/11/08 20:40:44 Done.
+ INITIALIZED,
+ // BrowserThread::ID is associated to a live thread.
+ RUNNING,
+ // BrowserThread::ID is associated to a thread having been or being shutdown.
+ SHUTDOWN
+};
+
struct BrowserThreadGlobals {
BrowserThreadGlobals()
: blocking_pool(
new base::SequencedWorkerPool(3,
"BrowserBlocking",
base::TaskPriority::USER_VISIBLE)) {
- memset(threads, 0, BrowserThread::ID_COUNT * sizeof(threads[0]));
- memset(thread_ids, 0, BrowserThread::ID_COUNT * sizeof(thread_ids[0]));
+ memset(states, 0, BrowserThread::ID_COUNT * sizeof(states[0]));
memset(thread_delegates, 0,
BrowserThread::ID_COUNT * sizeof(thread_delegates[0]));
Avi (use Gerrit) 2016/11/08 16:16:15 Does xxx = { 0 } syntax suffice to zero these? I'm
gab 2016/11/08 20:40:44 I just tried = {}; as a member initializer and it
}
- // This lock protects |threads| and |thread_ids|. Do not read or modify those
+ // This lock protects |task_runners| and |states|. Do not read or modify those
// arrays without holding this lock. Do not block while holding this lock.
base::Lock lock;
- // This array is protected by |lock|. IDs in this array are populated as soon
- // as their respective thread is started and are never reset.
- base::PlatformThreadId thread_ids[BrowserThread::ID_COUNT];
+ // This array is filled either as the underlying threads start and invoke
+ // Init() or in RedirectThreadIDToTaskRunner() for threads that are being
+ // redirected. It is not emptied during shutdown in order to support
+ // RunsTasksOnCurrentThread() until the very end.
+ scoped_refptr<base::SingleThreadTaskRunner>
+ task_runners[BrowserThread::ID_COUNT];
- // This array is protected by |lock|. The threads are not owned by this
- // array. Typically, the threads are owned on the UI thread by
- // BrowserMainLoop. BrowserThreadImpl objects remove themselves from this
- // array upon destruction.
- BrowserThreadImpl* threads[BrowserThread::ID_COUNT];
+ // Holds the state of each BrowserThread::ID. This needs to be a separate bit
+ // because |threads| isn't a good signal per redirected IDs remaining null and
+ // |task_runners| isn't either per not being cleaned up on shutdown.
+ BrowserThreadState states[BrowserThread::ID_COUNT];
// Only atomic operations are used on this array. The delegates are not owned
// by this array, rather by whoever calls BrowserThread::SetDelegate.
@@ -140,13 +157,6 @@ base::LazyInstance<BrowserThreadGlobals>::Leaky
BrowserThreadImpl::BrowserThreadImpl(ID identifier)
: Thread(GetThreadName(identifier)), identifier_(identifier) {
Initialize();
-
- // Unit tests may create multiple TestBrowserThreadBundles, causing the same
- // BrowserThread ID to be reinitialized. We explicitly clear the thread ID
- // here so Start() can sanity check.
- BrowserThreadGlobals& globals = g_globals.Get();
- base::AutoLock lock(globals.lock);
- globals.thread_ids[identifier] = base::kInvalidThreadId;
}
BrowserThreadImpl::BrowserThreadImpl(ID identifier,
@@ -159,7 +169,12 @@ BrowserThreadImpl::BrowserThreadImpl(ID identifier,
// which runs on the current thread.
BrowserThreadGlobals& globals = g_globals.Get();
base::AutoLock lock(globals.lock);
- globals.thread_ids[identifier] = base::PlatformThread::CurrentId();
+
+ DCHECK(!globals.task_runners[identifier_]);
+ globals.task_runners[identifier_] = message_loop->task_runner();
+
+ DCHECK_EQ(globals.states[identifier_], BrowserThreadState::INITIALIZED);
+ globals.states[identifier_] = BrowserThreadState::RUNNING;
}
// static
@@ -185,14 +200,25 @@ void BrowserThreadImpl::FlushThreadPoolHelperForTesting() {
void BrowserThreadImpl::Init() {
BrowserThreadGlobals& globals = g_globals.Get();
+ {
+ base::AutoLock lock(globals.lock);
+
+ // Grab the underlying thread's TaskRunner. This needs to happen right away
+ // as CurrentlyOn() depends on it.
+ globals.task_runners[identifier_] = message_loop()->task_runner();
+
+ DCHECK_EQ(globals.states[identifier_], BrowserThreadState::INITIALIZED);
+ globals.states[identifier_] = BrowserThreadState::RUNNING;
+ }
+
if (BrowserThread::CurrentlyOn(BrowserThread::DB) ||
BrowserThread::CurrentlyOn(BrowserThread::FILE) ||
BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING) ||
BrowserThread::CurrentlyOn(BrowserThread::PROCESS_LAUNCHER) ||
BrowserThread::CurrentlyOn(BrowserThread::CACHE)) {
- base::MessageLoop* message_loop = base::MessageLoop::current();
- message_loop->DisallowNesting();
- message_loop->DisallowTaskObservers();
+ // Nesting and task observers are not allowed on redirected threads.
+ message_loop()->DisallowNesting();
+ message_loop()->DisallowTaskObservers();
}
using base::subtle::AtomicWord;
@@ -314,7 +340,8 @@ void BrowserThreadImpl::CleanUp() {
// to prevent a race with accessing the message loop in PostTaskHelper(),
// remove this thread from the global array now.
base::AutoLock lock(globals.lock);
- globals.threads[identifier_] = nullptr;
+ DCHECK_EQ(globals.states[identifier_], BrowserThreadState::RUNNING);
+ globals.states[identifier_] = BrowserThreadState::SHUTDOWN;
}
void BrowserThreadImpl::Initialize() {
@@ -323,8 +350,8 @@ void BrowserThreadImpl::Initialize() {
base::AutoLock lock(globals.lock);
DCHECK_GE(identifier_, 0);
DCHECK_LT(identifier_, ID_COUNT);
- DCHECK_EQ(globals.threads[identifier_], nullptr);
- globals.threads[identifier_] = this;
+ DCHECK_EQ(globals.states[identifier_], BrowserThreadState::UNINITIALIZED);
+ globals.states[identifier_] = BrowserThreadState::INITIALIZED;
}
BrowserThreadImpl::~BrowserThreadImpl() {
@@ -335,38 +362,45 @@ BrowserThreadImpl::~BrowserThreadImpl() {
BrowserThreadGlobals& globals = g_globals.Get();
base::AutoLock lock(globals.lock);
- globals.threads[identifier_] = nullptr;
-#ifndef NDEBUG
+ // This thread should have gone through Cleanup() as part of Stop() and be in
+ // the SHUTDOWN state already (unless it uses an externally provided
+ // MessageLoop instead of a real underlying thread and thus doesn't go through
+ // Cleanup()).
+ if (using_external_message_loop_) {
+ DCHECK_EQ(globals.states[identifier_], BrowserThreadState::RUNNING);
+ globals.states[identifier_] = BrowserThreadState::SHUTDOWN;
+ } else {
+ DCHECK_EQ(globals.states[identifier_], BrowserThreadState::SHUTDOWN);
+ }
+#if DCHECK_IS_ON()
// Double check that the threads are ordered correctly in the enumeration.
for (int i = identifier_ + 1; i < ID_COUNT; ++i) {
- DCHECK(!globals.threads[i]) <<
- "Threads must be listed in the reverse order that they die";
+ DCHECK_EQ(globals.states[i], BrowserThreadState::SHUTDOWN)
+ << "Threads must be listed in the reverse order that they die";
}
#endif
}
-bool BrowserThreadImpl::Start() {
- return StartWithOptions(base::Thread::Options());
-}
-
-bool BrowserThreadImpl::StartWithOptions(const Options& options) {
- // The global thread table needs to be locked while a new thread is
- // starting, as the new thread can asynchronously start touching the
- // table (and other thread's message_loop).
+// static
+void BrowserThreadImpl::RedirectThreadIDToTaskRunner(
+ BrowserThread::ID identifier,
+ scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
BrowserThreadGlobals& globals = g_globals.Get();
base::AutoLock lock(globals.lock);
- DCHECK_EQ(globals.thread_ids[identifier_], base::kInvalidThreadId);
- bool result = Thread::StartWithOptions(options);
- globals.thread_ids[identifier_] = GetThreadId();
- return result;
+ if (task_runner) {
+ DCHECK(!globals.task_runners[identifier]);
+ DCHECK_EQ(globals.states[identifier], BrowserThreadState::UNINITIALIZED);
+
+ globals.task_runners[identifier] = std::move(task_runner);
+ globals.states[identifier] = BrowserThreadState::RUNNING;
+ } else {
+ // Cancelling redirection should only be done on previously redirected
+ // threads.
+ DCHECK_EQ(globals.states[identifier], BrowserThreadState::RUNNING);
+ globals.states[identifier] = BrowserThreadState::SHUTDOWN;
+ }
}
-bool BrowserThreadImpl::StartAndWaitForTesting() {
- if (!Start())
- return false;
- WaitUntilThreadStarted();
- return true;
-}
// static
bool BrowserThreadImpl::PostTaskHelper(
BrowserThread::ID identifier,
@@ -391,22 +425,23 @@ bool BrowserThreadImpl::PostTaskHelper(
if (!target_thread_outlives_current)
globals.lock.Acquire();
- base::MessageLoop* message_loop =
- globals.threads[identifier] ? globals.threads[identifier]->message_loop()
- : nullptr;
- if (message_loop) {
+ const bool accepting_tasks =
+ globals.states[identifier] == BrowserThreadState::RUNNING;
+ if (accepting_tasks) {
+ base::SingleThreadTaskRunner* task_runner =
+ globals.task_runners[identifier].get();
+ DCHECK(task_runner);
if (nestable) {
- message_loop->task_runner()->PostDelayedTask(from_here, task, delay);
+ task_runner->PostDelayedTask(from_here, task, delay);
} else {
- message_loop->task_runner()->PostNonNestableDelayedTask(from_here, task,
- delay);
+ task_runner->PostNonNestableDelayedTask(from_here, task, delay);
}
}
if (!target_thread_outlives_current)
globals.lock.Release();
- return !!message_loop;
+ return accepting_tasks;
}
// static
@@ -457,7 +492,8 @@ bool BrowserThread::IsThreadInitialized(ID identifier) {
base::AutoLock lock(globals.lock);
DCHECK_GE(identifier, 0);
DCHECK_LT(identifier, ID_COUNT);
- return globals.threads[identifier] != nullptr;
+ return globals.states[identifier] == BrowserThreadState::INITIALIZED ||
+ globals.states[identifier] == BrowserThreadState::RUNNING;
}
// static
@@ -466,7 +502,8 @@ bool BrowserThread::CurrentlyOn(ID identifier) {
base::AutoLock lock(globals.lock);
DCHECK_GE(identifier, 0);
DCHECK_LT(identifier, ID_COUNT);
- return base::PlatformThread::CurrentId() == globals.thread_ids[identifier];
+ return globals.task_runners[identifier] &&
+ globals.task_runners[identifier]->RunsTasksOnCurrentThread();
}
// static
@@ -492,8 +529,7 @@ bool BrowserThread::IsMessageLoopValid(ID identifier) {
base::AutoLock lock(globals.lock);
DCHECK_GE(identifier, 0);
DCHECK_LT(identifier, ID_COUNT);
- return globals.threads[identifier] &&
- globals.threads[identifier]->message_loop();
+ return globals.states[identifier] == BrowserThreadState::RUNNING;
}
// static
@@ -547,7 +583,6 @@ bool BrowserThread::GetCurrentThreadIdentifier(ID* identifier) {
if (g_globals == nullptr)
return false;
- base::MessageLoop* cur_message_loop = base::MessageLoop::current();
BrowserThreadGlobals& globals = g_globals.Get();
// Profiler to track potential contention on |globals.lock|. This only does
// real work on canary and local dev builds, so the cost of having this here
@@ -555,9 +590,9 @@ bool BrowserThread::GetCurrentThreadIdentifier(ID* identifier) {
tracked_objects::ScopedTracker tracking_profile(FROM_HERE);
base::AutoLock lock(globals.lock);
for (int i = 0; i < ID_COUNT; ++i) {
- if (globals.threads[i] &&
- globals.threads[i]->message_loop() == cur_message_loop) {
- *identifier = globals.threads[i]->identifier_;
+ if (globals.task_runners[i] &&
+ globals.task_runners[i]->RunsTasksOnCurrentThread()) {
+ *identifier = static_cast<ID>(i);
return true;
}
}

Powered by Google App Engine
This is Rietveld 408576698