|
|
DescriptionExperiment 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.
Committed: https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e
Cr-Commit-Position: refs/heads/master@{#439139}
Patch Set 1 #Patch Set 2 : cleanup and put behind experiment #Patch Set 3 : content->variations DEPS and some tweaks #Patch Set 4 : replace ThreadTaskRunnerHandle usage with RunsTasksOnCurrentThread #Patch Set 5 : Introduce BrowserThreadGlobals::states and rm BrowserThreadGlobals::threads/thread_ids :) #Patch Set 6 : Cancel redirection on BrowserMainLoop shutdown #Patch Set 7 : Synchronously flush redirected TaskRunners on shutdown. #Patch Set 8 : Decline tasks posted after cancelling a redirection -- better shutdown simulation #Patch Set 9 : Fix globals.states check edge case #Patch Set 10 : rm content->variations DEPS, use ContentBrowserClient instead #Patch Set 11 : Rely on Thread::using_external_message_loop_ instead of hard-coding BrowserThread::UI #
Total comments: 10
Patch Set 12 : Allow UNINITIALIZED as well as SHUTDOWN for other threads (happens in unit tests) #Patch Set 13 : Add ResetGlobalsForTesting() and fix shutdown state in unittests #
Total comments: 1
Patch Set 14 : review:avi#36 #Patch Set 15 : Extract dependencies: 2487073005 & 2484283005 #Patch Set 16 : Adapt TestBrowserThread and TestBrowserThreadBundle #Patch Set 17 : update dependencies #Patch Set 18 : Handle unintentionally null MessageLoop* #Patch Set 19 : fix DCHECK in ~BrowserThreadImpl #Patch Set 20 : Rebase and re-consider threads RUNNING during their async initialization. #Patch Set 21 : Rebase on https://codereview.chromium.org/2487343005/ and disallow null msg loop again #Patch Set 22 : Extract unittest side-effects to https://codereview.chromium.org/2523093002/ #Patch Set 23 : rebase on r434093 #Patch Set 24 : rebase on https://codereview.chromium.org/2535043003/ #Patch Set 25 : rebase on r434987 #Patch Set 26 : update dependencies #Patch Set 27 : rm TODO #
Total comments: 39
Patch Set 28 : update dependencies #Patch Set 29 : update dependencies #Patch Set 30 : rebase on r436155 #Patch Set 31 : Add missing lock in BrowserThreadImpl::Init in DCHECK builds #Patch Set 32 : rebase on r436331 #Patch Set 33 : review:fdoray#80 #Patch Set 34 : merge and fix compile/naming nit #Patch Set 35 : rebase on https://codereview.chromium.org/2558943002 #Patch Set 36 : Hold the lock while starting!!! #
Total comments: 17
Patch Set 37 : rebase on r437246 #Patch Set 38 : review:robliao#130/fdoray#131 #Patch Set 39 : fix compile #Patch Set 40 : WaitUntilThreadStarted on Android?? #Patch Set 41 : rebase on r437591 #Patch Set 42 : update dependencies #Patch Set 43 : rebase on r438560 #Patch Set 44 : nice comment for forced WaitUntilThreadStarted() on Android #Patch Set 45 : rebase on r438809 #Patch Set 46 : remove WaitUntilThreadStarted on Android, makes Android test instrumentation hang, is previous issu… #Messages
Total messages: 229 (209 generated)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
Description was changed from ========== Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler BUG=653916 ========== to ========== Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. BUG=653916 ==========
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. BUG=653916 ========== to ========== Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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 for the same period that the matching thread would be live. - On shutdown, we block on each TaskRunner's queue being flushed one-by-one in the same order as in the no-redirection case. BUG=653916 ==========
Description was changed from ========== Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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 for the same period that the matching thread would be live. - On shutdown, we block on each TaskRunner's queue being flushed one-by-one in the same order as in the no-redirection case. BUG=653916 ========== to ========== Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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 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. BUG=653916 ==========
Description was changed from ========== Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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 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. BUG=653916 ========== to ========== Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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 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. BUG=653916 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng ==========
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
Description was changed from ========== Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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 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. BUG=653916 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng ========== to ========== Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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 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. BUG=653916 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. ==========
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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 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. BUG=653916 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. ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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 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. BUG=653916 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. ==========
Description was changed from ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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 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. BUG=653916 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. ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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. BUG=653916 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. ==========
Description was changed from ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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. BUG=653916 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. ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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. BUG=653916 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. ==========
gab@chromium.org changed reviewers: + avi@chromium.org
Avi PTAL, thanks!
Oh boy. This looks reasonable (and fun) but can you get more eyes on this than just mine? I don't have a ton of threading experience and would appreciate more coverage. Mostly minor issues. https://codereview.chromium.org/2464233002/diff/200001/base/threading/thread.h File base/threading/thread.h (right): https://codereview.chromium.org/2464233002/diff/200001/base/threading/thread.... base/threading/thread.h:250: bool using_external_message_loop_ = false; Data members that are protected are a violation of the style guide (https://google.github.io/styleguide/cppguide.html#Access_Control). Use accessor functions. https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... content/browser/browser_main_loop.cc:319: DCHECK(!thread || !task_runner); This checks that we don't have both. Do we want to check that we have exactly one? (and below too) https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... File content/browser/browser_main_loop.h (right): https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... content/browser/browser_main_loop.h:276: std::unique_ptr<BrowserProcessSubThread> db_thread_; Can you add commentary to this block as to when the "thread_" or "task_runner_" variants are active? https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... content/browser/browser_thread_impl.cc:110: // BrowserThread::ID is associated but the underlying thread doesn't run yet. "... underlying thread hasn't run yet." https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... content/browser/browser_thread_impl.cc:126: BrowserThread::ID_COUNT * sizeof(thread_delegates[0])); Does xxx = { 0 } syntax suffice to zero these? I'm not a fan of memset. :\
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/11/08 16:16:15, Avi wrote: > Oh boy. This looks reasonable (and fun) but can you get more eyes on this than > just mine? I don't have a ton of threading experience and would appreciate more > coverage. Thanks for the initial look. I was going to send it to jam@ but he appears to have vanished for the next month, he told me that any content/OWNERS would do but I understand that some are more familiar with some parts than others. Any idea who in content/OWNERS would be a better fit for this review? PS: I'm still having trouble with a few tests so please hold off on a deeper review for now. https://codereview.chromium.org/2464233002/diff/200001/base/threading/thread.h File base/threading/thread.h (right): https://codereview.chromium.org/2464233002/diff/200001/base/threading/thread.... base/threading/thread.h:250: bool using_external_message_loop_ = false; On 2016/11/08 16:16:15, Avi wrote: > Data members that are protected are a violation of the style guide > (https://google.github.io/styleguide/cppguide.html#Access_Control). Use accessor > functions. Done. https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... content/browser/browser_main_loop.cc:319: DCHECK(!thread || !task_runner); On 2016/11/08 16:16:15, Avi wrote: > This checks that we don't have both. > > Do we want to check that we have exactly one? (and below too) Right, made this DCHECK((thread && !task_runner) || (!thread && task_runner)); instead. Otherwise, to verify inline below would require making the else {} and else if and having a NOTREACHED() else {} IMO which is uglier I think. https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... File content/browser/browser_main_loop.h (right): https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... content/browser/browser_main_loop.h:276: std::unique_ptr<BrowserProcessSubThread> db_thread_; On 2016/11/08 16:16:15, Avi wrote: > Can you add commentary to this block as to when the "thread_" or "task_runner_" > variants are active? Done. https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... content/browser/browser_thread_impl.cc:110: // BrowserThread::ID is associated but the underlying thread doesn't run yet. On 2016/11/08 16:16:15, Avi wrote: > "... underlying thread hasn't run yet." Done. https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse... content/browser/browser_thread_impl.cc:126: BrowserThread::ID_COUNT * sizeof(thread_delegates[0])); On 2016/11/08 16:16:15, Avi wrote: > Does xxx = { 0 } syntax suffice to zero these? I'm not a fan of memset. :\ I just tried = {}; as a member initializer and it seems to work (confirmed with runtime DCHECKs in concstructor locally -- and in practice the DCHECKs in SetDelegate() and BrowserThreadImpl() would also fire if this didn't work so I didn't keep the ones in constructor) :-)! PS: { 0 } just means "initialize the first entry to zero and default init the rest", {} means "default init everything" (I was also shocked when I learned this ;-)). https://codereview.chromium.org/2464233002/diff/240001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2464233002/diff/240001/content/browser/browse... content/browser/browser_main_loop.cc:326: FlushThreadTaskRunner(std::move(task_runner)); fdoray@ just made me realize offline that although we null out the redirection before doing the synchronous wait in FlushThreadTaskRunner(), which prevents BrowserThread::PostTask() to that ID from then on; it's still possible to post to that thread with ThreadTaskRunnerHandle::Get() -- and thus post something which will be queued after the Signal(). This is different from the current MessageLoop world which joins until the queue is idle. We have an idea to augment TaskScheduler to allow for this use case (e.g. SchedulerTaskRunner::RunUntilIdle()) and will come back to this CL once that's been implemented.
I'll OK things for content, but if you know other threads experts (content owners or not) then definitely get their eyes on this.
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #18 (id:340001) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #23 (id:460001) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #24 (id:500001) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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. BUG=653916 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. ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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. BUG=653916 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. ==========
gab@chromium.org changed reviewers: + fdoray@chromium.org, robliao@chromium.org
@fdoray/robliao PTAL at threading logic @danakj (optional -- I know you're swamped these days but if you find the time, I'd really like your threading expert eyes on this) PS: There are a few more red bots but it's just more incorrect destruction order for TestBrowserThreads (e.g. https://codereview.chromium.org/2535043003/) for a few things that don't run on Windows and I'll fix those in parallel with the review. Thanks! Gab
https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:327: } else { You could avoid keeping SingleThreadTaskRunners references in BrowserMainLoop by changing this code to: auto task_runner = BrowserThreadImpl::RedirectThreadIDToTaskRunner( BrowserThread::DB); FlushThreadTaskRunner(std::move(task_runner)); (assuming that RedirectThreadIDToTaskRunner() returns the previous TaskRunner for the provided identifier) https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:328: BrowserThreadImpl::RedirectThreadIDToTaskRunner(BrowserThread::DB, nullptr); Without redirection: 1. Main thread: ~BrowserProcessSubThread 2. Main thread: Thread::Stop() 3. Main thread: A QuitWhenIdle() task is posted to the thread. 4. Browser thread: Runs tasks until there are no more ready tasks. 5. Browser thread: Thread::Cleanup() 6. Browser thread: BrowserThreadImpl::CleanUp() sets the state to SHUTDOWN. No more tasks can be posted to the thread. 7. Browser thread: Exits, unblocking the join call on the main thread. With redirection: 1. Main thread: RedirectThreadIDToTaskRunner() sets the state to SHUTDOWN. No more tasks can be posted to the thread via the BrowserThread API. 2. Main thread: FlushThreadTaskRunner() posts a WaitableEvent::Signal() task to the browser thread and waits. 3. Browser thread: Runs all tasks that were posted before the call to FlushThreadTaskRunner() and unblocks the main thread. Without redirection, we wait for pending tasks to run and then we set the state to SHUTDOWN. With redirection, we set the state to SHUTDOWN and then we wait for tasks to run. Could we improve that? https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:942: // set to the appropriate |BrowserProcessSubThread*|. And |options| can be s/|BrowserProcessSubThread*|/BrowserProcessSubThread* https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:949: // |SingleThreadTaskRunner*| and |task_runner_traits| can be updated away s/|SingleThreadTaskRunner*|/SingleThreadTaskRunner* https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:950: // from its default. |execution_mode| defaults to |SINGLE_THREADED| as-is No more |execution_mode|. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:957: const bool redirect_nonUInonIO_browser_threads = Move outside the loop. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:969: task_runner_traits.WithFileIO() Add WithWait() to allow AssertIOAllowed()? https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:1081: } else if (task_runner_to_create) { DCHECK(!thread_to_start); https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:1244: // Clean up state that lives on or uses the file_thread_ before it goes the FILE thread https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:18: #include "base/task_scheduler/task_scheduler.h" Not needed. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:109: // BrowserThread::ID is associated but the underlying thread hasn't run yet. The INITIALIZED state is useless. See comment in BrowserThreadImpl::Initialize(). https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:111: // BrowserThread::ID is associated to a live thread. ... is associated to TaskRunner and is accepting tasks. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:113: // BrowserThread::ID is associated to a thread having been or being shutdown. ... no longer accepts tasks. If it was backed by a BrowserThreadImpl, this BrowserThreadImpl is being or has been shutdown. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:136: // because |threads| isn't a good signal per redirected IDs remaining null and |threads| doesn't exist https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:330: // PostTaskHelper() accesses the message loop while holding this lock. PostTaskHelper() no longer accesses a MessageLoop. Change to: // Change the state to SHUTDOWN so that PostTaskHelper stops // sending tasks to this thread. Do not clear // globals.task_runners[identifier_] so that // BrowserThread::CurrentlyOn() continues to work. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:346: globals.states[identifier_] = BrowserThreadState::INITIALIZED; You can get rid of the INITIALIZED state. Initialize() could be replaced with an anonymous function that verifies that the identifier it receives is valid. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:435: // threads. Add: // Change the state to SHUTDOWN so that PostTaskHelper stops // sending tasks to this thread. Do not clear // globals.task_runners[identifier] so that // BrowserThread::CurrentlyOn() continues to work. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:647: void BrowserThread::SetDelegate(ID identifier, I would change this to SetIOThreadDelegate() and change BrowserThreadGlobals::thread_delegates[] -> BrowserThreadGlobals::io_thread_delegate. The UI thread never supported delegates. Other threads don't support delegates when they are redirected. https://codereview.chromium.org/2464233002/diff/580001/content/public/test/te... File content/public/test/test_browser_thread.h (right): https://codereview.chromium.org/2464233002/diff/580001/content/public/test/te... content/public/test/test_browser_thread.h:55: BrowserThread::ID identifier_; const
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #30 (id:640001) has been deleted
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #30 (id:660001) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by gab@chromium.org
Patchset #30 (id:680001) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #33 (id:760001) has been deleted
@fdoray, comments addressed PTAL. Also, FYI, with a few more precursor CLs landed I'm now down to almost all bots passing :) https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:327: } else { On 2016/12/01 15:15:21, fdoray wrote: > You could avoid keeping SingleThreadTaskRunners references in BrowserMainLoop by > changing this code to: > > auto task_runner = BrowserThreadImpl::RedirectThreadIDToTaskRunner( > BrowserThread::DB); > FlushThreadTaskRunner(std::move(task_runner)); > > (assuming that RedirectThreadIDToTaskRunner() returns the previous TaskRunner > for the provided identifier) I was inspired by this and went a step further, the flush is now in BrowserThreadImpl::StopRedirectionOfThreadID() https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:328: BrowserThreadImpl::RedirectThreadIDToTaskRunner(BrowserThread::DB, nullptr); On 2016/12/01 15:15:21, fdoray wrote: > Without redirection: > 1. Main thread: ~BrowserProcessSubThread > 2. Main thread: Thread::Stop() > 3. Main thread: A QuitWhenIdle() task is posted to the thread. > 4. Browser thread: Runs tasks until there are no more ready tasks. > 5. Browser thread: Thread::Cleanup() > 6. Browser thread: BrowserThreadImpl::CleanUp() sets the state to SHUTDOWN. No > more tasks can be posted to the thread. > 7. Browser thread: Exits, unblocking the join call on the main thread. > > With redirection: > 1. Main thread: RedirectThreadIDToTaskRunner() sets the state to SHUTDOWN. No > more tasks can be posted to the thread via the BrowserThread API. > 2. Main thread: FlushThreadTaskRunner() posts a WaitableEvent::Signal() task to > the browser thread and waits. > 3. Browser thread: Runs all tasks that were posted before the call to > FlushThreadTaskRunner() and unblocks the main thread. > > Without redirection, we wait for pending tasks to run and then we set the state > to SHUTDOWN. With redirection, we set the state to SHUTDOWN and then we wait for > tasks to run. Could we improve that? The thing is we have to change the state to SHUTDOWN in Cleanup() because it always happens when the thread is stopped (which can happen outside of ~BrowserThreadImpl()) and since I want all state changes to DCHECK the previous state for documentation it can't also be toggled to SHUTDOWN before invoking Stop() in ~BrowserThreadImpl() -- and vice-versa, the thread could be SHUTDOWN before ~BrowserThreadImpl(). Thinking about this some more I think it's fine as-is as: 1) Any tasks for which this matters is already racing in today's world as it has to be posted from a thread which can't be aware of the main thread's state (beyond, potentially, the fact that it's past MainMessageLoopRun()). i.e. such a task already gets denied a %age of the time as it's not always posted by the time the Join() starts and has therefore no guarantees of being posted before the Join() completes. So triggering the SHUTDOWN state earlier merely skews this race towards denying more of these tasks which seems fine to me. The WaitableEvent::Signal still matters though as it guarantees all tasks that were knowingly scheduled ahead of the Join do get to run before the Join completes. As discussed further offline, there is still a minor edge case where previously it was impossible for a thread lower in BrowserThread::ID to fail to post to a higher thread. Whereas there is now a race (e.g. it's possible for a PostTaskAndReply to come back to the "shutdown" thread and run after the "join" -- before the final TaskScheduler::Shutdown()). We have a solution for this but won't implement it in this CL as the race only exists behind the experiment and we'll fix it before making it default on trunk. Added comments to this effect in BrowserThreadImpl::StopRedirectionToThreadID(). https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:942: // set to the appropriate |BrowserProcessSubThread*|. And |options| can be On 2016/12/01 15:15:21, fdoray wrote: > s/|BrowserProcessSubThread*|/BrowserProcessSubThread* Done. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:949: // |SingleThreadTaskRunner*| and |task_runner_traits| can be updated away On 2016/12/01 15:15:21, fdoray wrote: > s/|SingleThreadTaskRunner*|/SingleThreadTaskRunner* Done. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:950: // from its default. |execution_mode| defaults to |SINGLE_THREADED| as-is On 2016/12/01 15:15:21, fdoray wrote: > No more |execution_mode|. Done. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:957: const bool redirect_nonUInonIO_browser_threads = On 2016/12/01 15:15:21, fdoray wrote: > Move outside the loop. Done. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:969: task_runner_traits.WithFileIO() On 2016/12/01 15:15:21, fdoray wrote: > Add WithWait() to allow AssertIOAllowed()? Assuming you mean "to allow AssertWaitAllowed()". Looks like only UI/IO threads DisallowWaiting() at the moment so I'll add WithWait() to the other ones for now with a TODO to look into removing it. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:1081: } else if (task_runner_to_create) { On 2016/12/01 15:15:21, fdoray wrote: > DCHECK(!thread_to_start); This is implicit in else block of if (thread_to_start) https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_main_loop.cc:1244: // Clean up state that lives on or uses the file_thread_ before it goes On 2016/12/01 15:15:21, fdoray wrote: > the FILE thread Done. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:18: #include "base/task_scheduler/task_scheduler.h" On 2016/12/01 15:15:21, fdoray wrote: > Not needed. Done. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:109: // BrowserThread::ID is associated but the underlying thread hasn't run yet. On 2016/12/01 15:15:22, fdoray wrote: > The INITIALIZED state is useless. See comment in > BrowserThreadImpl::Initialize(). It is useful, replied there. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:111: // BrowserThread::ID is associated to a live thread. On 2016/12/01 15:15:22, fdoray wrote: > ... is associated to TaskRunner and is accepting tasks. Done. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:113: // BrowserThread::ID is associated to a thread having been or being shutdown. On 2016/12/01 15:15:21, fdoray wrote: > ... no longer accepts tasks. If it was backed by a BrowserThreadImpl, this > BrowserThreadImpl is being or has been shutdown. Done (without second sentence). https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:136: // because |threads| isn't a good signal per redirected IDs remaining null and On 2016/12/01 15:15:22, fdoray wrote: > |threads| doesn't exist Done. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:330: // PostTaskHelper() accesses the message loop while holding this lock. On 2016/12/01 15:15:22, fdoray wrote: > PostTaskHelper() no longer accesses a MessageLoop. Change to: > > // Change the state to SHUTDOWN so that PostTaskHelper stops > // sending tasks to this thread. Do not clear > // globals.task_runners[identifier_] so that > // BrowserThread::CurrentlyOn() continues to work. Done (slightly reworded). https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:346: globals.states[identifier_] = BrowserThreadState::INITIALIZED; On 2016/12/01 15:15:21, fdoray wrote: > You can get rid of the INITIALIZED state. Initialize() could be replaced with an > anonymous function that verifies that the identifier it receives is valid. No, INITIALIZED is required to support BrowserThread::IsThreadInitialized() versus BrowserThread::IsMessageLoopValid(). I don't know that it ultimately makes sense to have these two methods but such is the world we're dealing with and I'd rather have a 1:1 mapping for this CL. https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:435: // threads. On 2016/12/01 15:15:22, fdoray wrote: > Add: > > // Change the state to SHUTDOWN so that PostTaskHelper stops > // sending tasks to this thread. Do not clear > // globals.task_runners[identifier] so that > // BrowserThread::CurrentlyOn() continues to work. Detailed comments added to BrowserThreadImpl::StopRedirectionOfThreadID(). https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:647: void BrowserThread::SetDelegate(ID identifier, On 2016/12/01 15:15:22, fdoray wrote: > I would change this to SetIOThreadDelegate() and change > BrowserThreadGlobals::thread_delegates[] -> > BrowserThreadGlobals::io_thread_delegate. The UI thread never supported > delegates. Other threads don't support delegates when they are redirected. Good call, will do in precursor CL. https://codereview.chromium.org/2464233002/diff/580001/content/public/test/te... File content/public/test/test_browser_thread.h (right): https://codereview.chromium.org/2464233002/diff/580001/content/public/test/te... content/public/test/test_browser_thread.h:55: BrowserThread::ID identifier_; On 2016/12/01 15:15:22, fdoray wrote: > const Done.
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #36 (id:840001) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #36 (id:860001) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Looks reasonable. A few questions and comments. https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_main_loop.cc:928: TRACE_EVENT_BEGIN1("startup", Does it still make sense to do these TRACE_EVENT_BEGIN1's if we're going to redirect the thread? https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_thread_impl.cc:411: globals.states[identifier_] = BrowserThreadState::RUNNING; In the previous code, if thread creation failed, we hang forever (GetThreadId appears to wait for ThreadMain to signal its event). In this case, we mark the thread as running and proceed. Is this okay? https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_thread_impl.cc:514: base::SingleThreadTaskRunner* task_runner = Since we don't always lock here, it seems like it's possible for a task to get posted after the flush event signal task above. Maybe we should lock now? https://codereview.chromium.org/2464233002/diff/860002/content/public/test/te... File content/public/test/test_browser_thread.cc (right): https://codereview.chromium.org/2464233002/diff/860002/content/public/test/te... content/public/test/test_browser_thread.cc:57: // Resets BrowserThreadImpl globals binding |identifier_| to |impl_|. This is How does the below set the binding from |identifier_| to |impl_|? ResetGlobalForTesting looks like it just clears everything. https://codereview.chromium.org/2464233002/diff/860002/content/public/test/te... content/public/test/test_browser_thread.cc:65: // should instead use this shutdown sequence: TestBrowserThread::Stop() -> Nit: Might be easier to read this as a sequenced list.
lgtm w/ comments https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_main_loop.cc:928: TRACE_EVENT_BEGIN1("startup", On 2016/12/08 01:57:17, robliao wrote: > Does it still make sense to do these TRACE_EVENT_BEGIN1's if we're going to > redirect the thread? Before, StartWithOptions blocked until the thread was started https://cs.chromium.org/chromium/src/content/browser/browser_thread_impl.cc?l... Now, it returns immediately https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... . Hence, the duration of this trace event should be very short with or without redirection. https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_main_loop.cc:958: // On Windows, the FILE thread needs to be have a UI message loop which needs to be have -> remove be https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_thread_impl.cc:403: // Although the thread is starting asynchronously. The MessageLoop is already Although the thread is starting asynchronously <<,>> the https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_thread_impl.cc:411: globals.states[identifier_] = BrowserThreadState::RUNNING; On 2016/12/08 01:57:17, robliao wrote: > In the previous code, if thread creation failed, we hang forever (GetThreadId > appears to wait for ThreadMain to signal its event). > > In this case, we mark the thread as running and proceed. Is this okay? This change could make startup faster!
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop and id in favor of TaskRunners and BrowserThreadState. 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. BUG=653916 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. ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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 :). 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. BUG=653916 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. ==========
Description was changed from ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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 :). 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. BUG=653916 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. ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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 :). 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 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. ==========
Description was changed from ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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 :). 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 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. ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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. 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 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. ==========
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Done PTAnL, thanks! https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse... content/browser/browser_thread_impl.cc:647: void BrowserThread::SetDelegate(ID identifier, On 2016/12/07 19:15:27, gab wrote: > On 2016/12/01 15:15:22, fdoray wrote: > > I would change this to SetIOThreadDelegate() and change > > BrowserThreadGlobals::thread_delegates[] -> > > BrowserThreadGlobals::io_thread_delegate. The UI thread never supported > > delegates. Other threads don't support delegates when they are redirected. > > Good call, will do in precursor CL. Done in http://crrev.com/437271 :) https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_main_loop.cc:928: TRACE_EVENT_BEGIN1("startup", On 2016/12/08 14:51:46, fdoray wrote: > On 2016/12/08 01:57:17, robliao wrote: > > Does it still make sense to do these TRACE_EVENT_BEGIN1's if we're going to > > redirect the thread? > > Before, StartWithOptions blocked until the thread was started > https://cs.chromium.org/chromium/src/content/browser/browser_thread_impl.cc?l... > Now, it returns immediately > https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... > . Hence, the duration of this trace event should be very short with or without > redirection. Right, it should be short, but I don't think it hurts to have them. We can strip them later in the follow-up CL when making this the default. In this CL I'm merely looking to keep status quo. https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_main_loop.cc:958: // On Windows, the FILE thread needs to be have a UI message loop which On 2016/12/08 14:51:46, fdoray wrote: > needs to be have -> remove be Done. https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_thread_impl.cc:403: // Although the thread is starting asynchronously. The MessageLoop is already On 2016/12/08 14:51:46, fdoray wrote: > Although the thread is starting asynchronously <<,>> the Done. https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_thread_impl.cc:411: globals.states[identifier_] = BrowserThreadState::RUNNING; On 2016/12/08 14:51:46, fdoray wrote: > On 2016/12/08 01:57:17, robliao wrote: > > In the previous code, if thread creation failed, we hang forever (GetThreadId > > appears to wait for ThreadMain to signal its event). > > > > In this case, we mark the thread as running and proceed. Is this okay? Yes this is fine because browser_main_loop.cc does this: if (!(*thread_to_start)->StartWithOptions(options)) LOG(FATAL) << "Failed to start the browser thread: id == " << id; > > This change could make startup faster! Exactly, I was surprised to see that we were blocking on getting the ID here, the whole point of making the start asynchronous a year or two ago was to avoid blocking on threads that are launching (i.e. it's fine to let them start async so long as we make their ML/TaskRunners available the callers can't notice the difference). Looks like this was an unintentional regression from http://crrev.com/408295. So we're back to an asynchronous thread start, yay :). Updated CL desc to highlight this. https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_thread_impl.cc:514: base::SingleThreadTaskRunner* task_runner = On 2016/12/08 01:57:17, robliao wrote: > Since we don't always lock here, it seems like it's possible for a task to get > posted after the flush event signal task above. Maybe we should lock now? There is indeed a theoretical race as described in BrowserThreadImpl::StopRedirectionOfThreadID() because a task posted to a flushed thread A can theoretically run after a thread B that is meant to die after thread A and B can thus unexpectedly have been shutdown ahead of a task on A. This theoretical race is only there on shutdown when the experiment is enabled. The better solution long-term is to join IO thread, then TaskScheduler::Shutdown() then destroy BrowserThreadImpl for UI thread. But this requires that all other threads be redirected or we still have ordering problems. Solving the race in the meantime either requires always locking here (which breaks my intent of this CL being status quo) or truly mimic'ing status quo by introducing a Join on task_scheduler::Sequence which is a pain for something we don't want long-term. So we figured we were okay with a theoretical shutdown race in the initial experiment (there's a TODO in BrowserThreadImpl::StopRedirectionOfThreadID() to fix this before making it default). https://codereview.chromium.org/2464233002/diff/860002/content/public/test/te... File content/public/test/test_browser_thread.cc (right): https://codereview.chromium.org/2464233002/diff/860002/content/public/test/te... content/public/test/test_browser_thread.cc:57: // Resets BrowserThreadImpl globals binding |identifier_| to |impl_|. This is On 2016/12/08 01:57:17, robliao wrote: > How does the below set the binding from |identifier_| to |impl_|? > > ResetGlobalForTesting looks like it just clears everything. I meant to say that it clears that very binding, reworded. https://codereview.chromium.org/2464233002/diff/860002/content/public/test/te... content/public/test/test_browser_thread.cc:65: // should instead use this shutdown sequence: TestBrowserThread::Stop() -> On 2016/12/08 01:57:17, robliao wrote: > Nit: Might be easier to read this as a sequenced list. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
The CQ bit was unchecked by gab@chromium.org
Description was changed from ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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. 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 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. ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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. 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 # Would be nice but bot is too flaky at the moment... # 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. ==========
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by gab@chromium.org
The CQ bit was checked by gab@chromium.org to run a CQ dry run
The CQ bit was unchecked by gab@chromium.org
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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. 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 # Would be nice but bot is too flaky at the moment... # 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. ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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. 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 # Would be nice but bot is too flaky at the moment... # 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. ==========
Description was changed from ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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. 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 # Would be nice but bot is too flaky at the moment... # 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. ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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 # Would be nice but bot is too flaky at the moment... # 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. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #42 (id:990001) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by gab@chromium.org
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse... content/browser/browser_thread_impl.cc:411: globals.states[identifier_] = BrowserThreadState::RUNNING; On 2016/12/08 18:49:41, gab wrote: > On 2016/12/08 14:51:46, fdoray wrote: > > On 2016/12/08 01:57:17, robliao wrote: > > > In the previous code, if thread creation failed, we hang forever > (GetThreadId > > > appears to wait for ThreadMain to signal its event). > > > > > > In this case, we mark the thread as running and proceed. Is this okay? > > Yes this is fine because browser_main_loop.cc does this: > > if (!(*thread_to_start)->StartWithOptions(options)) > LOG(FATAL) << "Failed to start the browser thread: id == " << id; > > > > > This change could make startup faster! > > Exactly, I was surprised to see that we were blocking on getting the ID here, > the whole point of making the start asynchronous a year or two ago was to avoid > blocking on threads that are launching (i.e. it's fine to let them start async > so long as we make their ML/TaskRunners available the callers can't notice the > difference). Looks like this was an unintentional regression from > http://crrev.com/408295. > > So we're back to an asynchronous thread start, yay :). > > Updated CL desc to highlight this. The state seems a little wonky given that we enter RUNNING even on failure, but given we check on the outside, I think I can live with that.
gab@chromium.org changed reviewers: + jam@chromium.org
Thanks, @avi for content (+jam who's had time to come back before everything this tripped was fixed..!)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #43 (id:1030001) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM good luck
The CQ bit was unchecked by gab@chromium.org
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org, robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2464233002/#ps1070001 (title: "nice comment for forced WaitUntilThreadStarted() on Android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gab@chromium.org changed reviewers: - jam@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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 # Would be nice but bot is too flaky at the moment... # 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. ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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. ==========
Patchset #46 (id:1110001) has been deleted
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, avi@chromium.org, fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2464233002/#ps1130001 (title: "remove WaitUntilThreadStarted on Android, makes Android test instrumentation hang, is previous issu…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 1130001, "attempt_start_ts": 1481905639694660, "parent_rev": "26d22903b6eb33b154ee5d8ce232a0fe73f6e52d", "commit_rev": "46377e449f09ecb45c5e41cc89fce609bf9cbbbc"}
Message was sent while issue was closed.
Description was changed from ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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. ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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 ==========
Message was sent while issue was closed.
Committed patchset #46 (id:1130001)
Message was sent while issue was closed.
Description was changed from ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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 ========== to ========== 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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... 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. Committed: https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e Cr-Commit-Position: refs/heads/master@{#439139} ==========
Message was sent while issue was closed.
Patchset 46 (id:??) landed as https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e Cr-Commit-Position: refs/heads/master@{#439139} |