Commit 3ee0e445 authored by gab's avatar gab Committed by Commit bot

Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler

This is part of the migration phase for TaskScheduler, design doc:
https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa5k/edit

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop
and id in favor of TaskRunners and BrowserThreadState.

Cool side-effect: BrowserThreadImpl::StartWithOptions() no longer blocks on the
underlying thread starting (it used to via GetThreadId()), it now takes a reference
to its TaskRunner and lets the thread itself start on its own schedule :).
This wasn't previously intended to block (BrowserThreadImpl::StartAndWaitForTesting() is
supposed to be used for that) but had unintentionally regressed in http://crrev.com/408295
because GetThreadId() implicitly waits (http://crbug.com/672977).

Another nice improvement is that ~BrowserThreadImpl() now cleans the BrowserThreadGlobals
associated with it, this has the side-effect to force proper destruction order of
TestBrowserThread in tests which brought forth a few precursor cleanups
(https://crbug.com/653916#c24) which will from now on be enforced by this CL.

When redirection is disabled, the logic should be the exact same as before.
 - Threads are brought up.
 - Tasks are posted to their MessageLoop (albeit through their TaskRunner now).
 - On shutdown, threads are joined one by one.

When redirection is enabled, we try to mimic this logic.
 - Redirection TaskRunners are only live (|accepting_tasks|) for the same period that the
   matching thread would be.
 - On shutdown, we block on each TaskRunner's queue being flushed one-by-one
   in the same order as in the no-redirection case (this almost identical to
   real threads join % one slight difference documented in detail in
   BrowserThreadImpl::StopRedirectionOfThreadID()).

BUG=653916, 672977
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng

TEST=
 A) "out\Release\chrome.exe"
    Runs/shuts down the exact same as before.

 B) "out\Release\chrome.exe --force-fieldtrials=BrowserScheduler/Enabled/ --force-fieldtrial-params=BrowserScheduler.Enabled:Background/3;3;1;0;10000/BackgroundFileIO/3;3;1;0;10000/Foreground/3;3;1;0;10000/ForegroundFileIO/3;3;1;0;10000/RedirectSequencedWorkerPools/true/RedirectNonUINonIOBrowserThreads/true"
    Runs/shuts down smoothly and chrome://tracing confirms that redirected BrowserThreads are running on TaskSchedulerWorkers.

Review-Url: https://codereview.chromium.org/2464233002
Cr-Commit-Position: refs/heads/master@{#439139}
parent 14654062
...@@ -3372,7 +3372,13 @@ void ChromeContentBrowserClient:: ...@@ -3372,7 +3372,13 @@ void ChromeContentBrowserClient::
MaybePerformBrowserTaskSchedulerRedirection(); MaybePerformBrowserTaskSchedulerRedirection();
} }
//static bool ChromeContentBrowserClient::
RedirectNonUINonIOBrowserThreadsToTaskScheduler() {
return variations::GetVariationParamValue(
"BrowserScheduler", "RedirectNonUINonIOBrowserThreads") == "true";
}
// static
void ChromeContentBrowserClient::SetDefaultQuotaSettingsForTesting( void ChromeContentBrowserClient::SetDefaultQuotaSettingsForTesting(
const storage::QuotaSettings* settings) { const storage::QuotaSettings* settings) {
g_default_quota_settings = settings; g_default_quota_settings = settings;
......
...@@ -324,8 +324,8 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient { ...@@ -324,8 +324,8 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
std::vector<base::SchedulerWorkerPoolParams>* params_vector, std::vector<base::SchedulerWorkerPoolParams>* params_vector,
base::TaskScheduler::WorkerPoolIndexForTraitsCallback* base::TaskScheduler::WorkerPoolIndexForTraitsCallback*
index_to_traits_callback) override; index_to_traits_callback) override;
void PerformExperimentalTaskSchedulerRedirections() override; void PerformExperimentalTaskSchedulerRedirections() override;
bool RedirectNonUINonIOBrowserThreadsToTaskScheduler() override;
private: private:
friend class DisableWebRtcEncryptionFlagTest; friend class DisableWebRtcEncryptionFlagTest;
......
This diff is collapsed.
...@@ -26,10 +26,10 @@ namespace base { ...@@ -26,10 +26,10 @@ namespace base {
class CommandLine; class CommandLine;
class FilePath; class FilePath;
class HighResolutionTimerManager; class HighResolutionTimerManager;
class MemoryPressureMonitor;
class MessageLoop; class MessageLoop;
class PowerMonitor; class PowerMonitor;
class SystemMonitor; class SystemMonitor;
class MemoryPressureMonitor;
namespace trace_event { namespace trace_event {
class TraceEventSystemStatsMonitor; class TraceEventSystemStatsMonitor;
} // namespace trace_event } // namespace trace_event
...@@ -273,6 +273,9 @@ class CONTENT_EXPORT BrowserMainLoop { ...@@ -273,6 +273,9 @@ class CONTENT_EXPORT BrowserMainLoop {
#endif #endif
// Members initialized in |CreateThreads()| ---------------------------------- // Members initialized in |CreateThreads()| ----------------------------------
// Note: some |*_thread_| members below may never be initialized when
// redirection to TaskScheduler is enabled. (ref.
// ContentBrowserClient::RedirectNonUINonIOBrowserThreadsToTaskScheduler()).
std::unique_ptr<BrowserProcessSubThread> db_thread_; std::unique_ptr<BrowserProcessSubThread> db_thread_;
std::unique_ptr<BrowserProcessSubThread> file_user_blocking_thread_; std::unique_ptr<BrowserProcessSubThread> file_user_blocking_thread_;
std::unique_ptr<BrowserProcessSubThread> file_thread_; std::unique_ptr<BrowserProcessSubThread> file_thread_;
......
This diff is collapsed.
...@@ -5,7 +5,8 @@ ...@@ -5,7 +5,8 @@
#ifndef CONTENT_BROWSER_BROWSER_THREAD_IMPL_H_ #ifndef CONTENT_BROWSER_BROWSER_THREAD_IMPL_H_
#define CONTENT_BROWSER_BROWSER_THREAD_IMPL_H_ #define CONTENT_BROWSER_BROWSER_THREAD_IMPL_H_
#include "base/threading/platform_thread.h" #include "base/memory/ref_counted.h"
#include "base/single_thread_task_runner.h"
#include "base/threading/thread.h" #include "base/threading/thread.h"
#include "content/common/content_export.h" #include "content/common/content_export.h"
#include "content/public/browser/browser_thread.h" #include "content/public/browser/browser_thread.h"
...@@ -32,8 +33,26 @@ class CONTENT_EXPORT BrowserThreadImpl : public BrowserThread, ...@@ -32,8 +33,26 @@ class CONTENT_EXPORT BrowserThreadImpl : public BrowserThread,
bool StartWithOptions(const Options& options); bool StartWithOptions(const Options& options);
bool StartAndWaitForTesting(); bool StartAndWaitForTesting();
// Redirects tasks posted to |identifier| to |task_runner|.
static void RedirectThreadIDToTaskRunner(
BrowserThread::ID identifier,
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
// Makes this |identifier| no longer accept tasks and synchronously flushes
// any tasks previously posted to it.
// Can only be called after a matching RedirectThreadIDToTaskRunner call.
static void StopRedirectionOfThreadID(BrowserThread::ID identifier);
static void ShutdownThreadPool(); static void ShutdownThreadPool();
// Resets globals for |identifier|. Used in tests to clear global state that
// would otherwise leak to the next test. Globals are not otherwise fully
// cleaned up in ~BrowserThreadImpl() as there are subtle differences between
// UNINITIALIZED and SHUTDOWN state (e.g. globals.task_runners are kept around
// on shutdown). Must be called after ~BrowserThreadImpl() for the given
// |identifier|.
static void ResetGlobalsForTesting(BrowserThread::ID identifier);
protected: protected:
void Init() override; void Init() override;
void Run(base::RunLoop* run_loop) override; void Run(base::RunLoop* run_loop) override;
......
...@@ -441,4 +441,8 @@ ContentBrowserClient::GetMemoryCoordinatorDelegate() { ...@@ -441,4 +441,8 @@ ContentBrowserClient::GetMemoryCoordinatorDelegate() {
return nullptr; return nullptr;
} }
bool ContentBrowserClient::RedirectNonUINonIOBrowserThreadsToTaskScheduler() {
return false;
}
} // namespace content } // namespace content
...@@ -814,6 +814,10 @@ class CONTENT_EXPORT ContentBrowserClient { ...@@ -814,6 +814,10 @@ class CONTENT_EXPORT ContentBrowserClient {
// Performs any necessary PostTask API redirection to the task scheduler. // Performs any necessary PostTask API redirection to the task scheduler.
virtual void PerformExperimentalTaskSchedulerRedirections() {} virtual void PerformExperimentalTaskSchedulerRedirections() {}
// If this returns true, all BrowserThreads (but UI/IO) that support it on
// this platform will experimentally be redirected to TaskScheduler.
virtual bool RedirectNonUINonIOBrowserThreadsToTaskScheduler();
}; };
} // namespace content } // namespace content
......
...@@ -40,15 +40,34 @@ class TestBrowserThreadImpl : public BrowserThreadImpl { ...@@ -40,15 +40,34 @@ class TestBrowserThreadImpl : public BrowserThreadImpl {
}; };
TestBrowserThread::TestBrowserThread(BrowserThread::ID identifier) TestBrowserThread::TestBrowserThread(BrowserThread::ID identifier)
: impl_(new TestBrowserThreadImpl(identifier)) { : impl_(new TestBrowserThreadImpl(identifier)), identifier_(identifier) {}
}
TestBrowserThread::TestBrowserThread(BrowserThread::ID identifier, TestBrowserThread::TestBrowserThread(BrowserThread::ID identifier,
base::MessageLoop* message_loop) base::MessageLoop* message_loop)
: impl_(new TestBrowserThreadImpl(identifier, message_loop)) {} : impl_(new TestBrowserThreadImpl(identifier, message_loop)),
identifier_(identifier) {}
TestBrowserThread::~TestBrowserThread() { TestBrowserThread::~TestBrowserThread() {
Stop(); // The upcoming BrowserThreadImpl::ResetGlobalsForTesting() call requires that
// |impl_| have triggered the shutdown phase for its BrowserThread::ID. This
// either happens when the thread is stopped (if real) or destroyed (when fake
// -- i.e. using an externally provided MessageLoop).
impl_.reset();
// Resets BrowserThreadImpl's globals so that |impl_| is no longer bound to
// |identifier_|. This is fine since the underlying MessageLoop has already
// been flushed and deleted in Stop(). In the case of an externally provided
// MessageLoop however, this means that TaskRunners obtained through
// |BrowserThreadImpl::GetTaskRunnerForThread(identifier_)| will no longer
// recognize their BrowserThreadImpl for RunsTasksOnCurrentThread(). This
// happens most often when such verifications are made from
// MessageLoop::DestructionObservers. Callers that care to work around that
// should instead use this shutdown sequence:
// 1) TestBrowserThread::Stop()
// 2) ~MessageLoop()
// 3) ~TestBrowserThread()
// (~TestBrowserThreadBundle() does this).
BrowserThreadImpl::ResetGlobalsForTesting(identifier_);
} }
bool TestBrowserThread::Start() { bool TestBrowserThread::Start() {
......
...@@ -52,6 +52,8 @@ class TestBrowserThread { ...@@ -52,6 +52,8 @@ class TestBrowserThread {
private: private:
std::unique_ptr<TestBrowserThreadImpl> impl_; std::unique_ptr<TestBrowserThreadImpl> impl_;
const BrowserThread::ID identifier_;
DISALLOW_COPY_AND_ASSIGN(TestBrowserThread); DISALLOW_COPY_AND_ASSIGN(TestBrowserThread);
}; };
......
...@@ -4,6 +4,7 @@ ...@@ -4,6 +4,7 @@
#include "content/public/test/test_browser_thread_bundle.h" #include "content/public/test/test_browser_thread_bundle.h"
#include "base/logging.h"
#include "base/message_loop/message_loop.h" #include "base/message_loop/message_loop.h"
#include "base/run_loop.h" #include "base/run_loop.h"
#include "content/browser/browser_thread_impl.h" #include "content/browser/browser_thread_impl.h"
...@@ -27,29 +28,35 @@ TestBrowserThreadBundle::~TestBrowserThreadBundle() { ...@@ -27,29 +28,35 @@ TestBrowserThreadBundle::~TestBrowserThreadBundle() {
BrowserThreadImpl::FlushThreadPoolHelperForTesting(); BrowserThreadImpl::FlushThreadPoolHelperForTesting();
// To ensure a clean teardown, each thread's message loop must be flushed // To ensure a clean teardown, each thread's message loop must be flushed
// just before the thread is destroyed. But destroying a fake thread does not // just before the thread is destroyed. But stopping a fake thread does not
// automatically flush the message loop, so we have to do it manually. // automatically flush the message loop, so we have to do it manually.
// See http://crbug.com/247525 for discussion. // See http://crbug.com/247525 for discussion.
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
io_thread_.reset(); io_thread_->Stop();
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
cache_thread_.reset(); cache_thread_->Stop();
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
process_launcher_thread_.reset(); process_launcher_thread_->Stop();
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
file_user_blocking_thread_.reset(); file_user_blocking_thread_->Stop();
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
file_thread_.reset(); file_thread_->Stop();
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
db_thread_.reset(); db_thread_->Stop();
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
// This is the point at which we normally shut down the thread pool. So flush // This is the point at which we normally shut down the thread pool. So flush
// it again in case any shutdown tasks have been posted to the pool from the // it again in case any shutdown tasks have been posted to the pool from the
// threads above. // threads above.
BrowserThreadImpl::FlushThreadPoolHelperForTesting(); BrowserThreadImpl::FlushThreadPoolHelperForTesting();
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
ui_thread_.reset(); ui_thread_->Stop();
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
// |message_loop_| needs to explicitly go away before fake threads in order
// for DestructionObservers hooked to |message_loop_| to be able to invoke
// BrowserThread::CurrentlyOn() -- ref. ~TestBrowserThread().
CHECK(message_loop_->IsIdleForTesting());
message_loop_.reset();
} }
void TestBrowserThreadBundle::Init() { void TestBrowserThreadBundle::Init() {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment