Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2241703002:
Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. (Closed)
|
Created:
4 years, 4 months ago by gab Modified:
4 years, 3 months ago CC:
chromium-reviews, gab+watch_chromium.org, robliao+watch_chromium.org, fdoray+watch_chromium.org, robliao, fdoray, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@b3_delay_metrics_blocking Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd an experiment to redirect SequencedWorkerPool tasks to TaskScheduler.
The experiment is controlled by a static call for the entire process
which must be made before any SequencedWorkerPool instance is posted
a task (i.e. before it creates a Worker).
The experiment being at run-time, sequenced_worker_pool.cc is now divided
between the two worlds. DCHECKs were added to associate as many methods as
possible to the world they belong to.
FYI, the experiment (and TEST= below) will not actually work until
https://codereview.chromium.org/2237643002/ or a variant of it lands
but this is a stepping stone to getting there and the CLs
themselves can land in any order.
BUG=622400
TEST=
A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing
B) "out\Debug\chrome --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"
runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-)
Committed: https://crrev.com/5a65a6844ec5348feedb041cad8721ab79715395
Cr-Commit-Position: refs/heads/master@{#413346}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Split PostTask into helpers and add bug comments. #
Total comments: 14
Patch Set 3 : review:brettw/danakj #19-20 #Patch Set 4 : self-review #
Total comments: 10
Messages
Total messages: 41 (20 generated)
Description was changed from ========== Experiment that redirects SequencedWorkerPool tasks to TaskScheduler. BUG= ========== to ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
gab@chromium.org changed reviewers: + danakj@chromium.org
Dana PTAL. This is the most important review on the stack from me (besides stamping https://codereview.chromium.org/2204333003/ which will unblock 2 already lgtm'ed CLs which have to land before this one -- to get rid of the priority-less SequencedWorkerPool constructor). This finally lets us enable the TaskScheduler to pickup some real work on Canary :-)!!! There is a TODO left for me in the code and I also want to add parametrized tests to sequenced_worker_pool_unittest.cc to test both sides of the impl, but since this is behind a 0% experiment, I'd be fine landing this as-is (today is my last official day before my vacation and I would *really* like to land this so that my team has an actual live workload to play with while I'm away -- next step is adding metrics and diagnosis pages before ramping up the experiment for the public and it will be much easier for them to do that with a live workload). Thanks a gigalot for helping us get to this point :-)! Gab
gab@chromium.org changed reviewers: + sky@chromium.org
gab@chromium.org changed reviewers: + jam@chromium.org - sky@chromium.org
gab@chromium.org changed reviewers: + sky@chromium.org - jam@chromium.org
+sky for chrome_browser_main.cc CC jam/brettw as FYI, redirection to TaskScheduler is happening (albeit still opt-in only behind 0% experiment for just a tad longer :-)) On 2016/08/12 03:42:34, gab (OOO soon - Sep 1) wrote: > Dana PTAL. This is the most important review on the stack from me (besides > stamping https://codereview.chromium.org/2204333003/ which will unblock 2 > already lgtm'ed CLs which have to land before this one -- to get rid of the > priority-less SequencedWorkerPool constructor). > > This finally lets us enable the TaskScheduler to pickup some real work on Canary > :-)!!! > > There is a TODO left for me in the code and I also want to add parametrized > tests to sequenced_worker_pool_unittest.cc to test both sides of the impl, but > since this is behind a 0% experiment, I'd be fine landing this as-is (today is > my last official day before my vacation and I would *really* like to land this > so that my team has an actual live workload to play with while I'm away -- next > step is adding metrics and diagnosis pages before ramping up the experiment for > the public and it will be much easier for them to do that with a live workload). > > Thanks a gigalot for helping us get to this point :-)! > Gab
This change seems like it good have a big impact. By putting it behind an experiment we get no test coverage. Have you run perf numbers and submitted try jobs with this on by default?
On 2016/08/12 15:16:06, sky wrote: > This change seems like it good have a big impact. By putting it behind an > experiment we get no test coverage. Have you run perf numbers and submitted try > jobs with this on by default? Not yet, the plan is to have this behind a 0% experiment so that the TaskScheduler team can experiment live on Canary (e.g. try to trip SyzyASAN, etc.). We will definitely enable this temporarily (not permanently at first to make sure we keep coverage for the configuration we ship) on the waterfall and make sure everything is green before we start experimenting with any % of real users. For now we're still missing a few metrics in the TaskScheduler which make the experiment not extremely worthwhile, but this gets us a step closer by having all the logic in place for the TaskScheduler team to experiment with in the mean time in a live Canary.
brettw@chromium.org changed reviewers: + brettw@chromium.org
This is adding a lot of code for this experiment in the middle of a relatively complicated and critical piece of Chrome internals. I think it makes sense that we do experimentation here, but I think we can do a bit more work to lessen the code impact and make the experimental code easier to remove. I'm thinking: - File a bug and mark all code you're adding for the experiment with a reference to that bug. This way we can just search for that bug number to find code that should be removed when we're done experimenting. - Work to separate out the task scheduler forwarding code a bit more. PostTask is the main example where you're adding a lot of complicated code into an already long and complicated function. Can this be separated out more? https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:519: /* Members used for the experimental redirection to TaskScheduler. */ Can this be a normal // comment?
On 2016/08/12 17:44:39, brettw (ping on IM after 24h) wrote: > This is adding a lot of code for this experiment in the middle of a relatively > complicated and critical piece of Chrome internals. > > I think it makes sense that we do experimentation here, but I think we can do a > bit more work to lessen the code impact and make the experimental code easier to > remove. I'm thinking: > > - File a bug and mark all code you're adding for the experiment with a > reference to that bug. This way we can just search for that bug number to find > code that should be removed when we're done experimenting. This is the intent of the AllPoolsState DCHECKs which not only document which code runs in which mode but enforces it. > > - Work to separate out the task scheduler forwarding code a bit more. PostTask > is the main example where you're adding a lot of complicated code into an > already long and complicated function. Can this be separated out more? I could but a lot of the setup is the same, I guess I could forward the remainder of PostTask (after the common chunk) to separate helpers, WDYT? > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > File base/threading/sequenced_worker_pool.cc (right): > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > base/threading/sequenced_worker_pool.cc:519: /* Members used for the > experimental redirection to TaskScheduler. */ > Can this be a normal // comment? Will do.
On 2016/08/12 18:49:21, gab (OOO Aug 15 - Sep 1) wrote: > On 2016/08/12 17:44:39, brettw (ping on IM after 24h) wrote: > > This is adding a lot of code for this experiment in the middle of a relatively > > complicated and critical piece of Chrome internals. > > > > I think it makes sense that we do experimentation here, but I think we can do > a > > bit more work to lessen the code impact and make the experimental code easier > to > > remove. I'm thinking: > > > > - File a bug and mark all code you're adding for the experiment with a > > reference to that bug. This way we can just search for that bug number to find > > code that should be removed when we're done experimenting. > > This is the intent of the AllPoolsState DCHECKs which not only document which > code runs in which mode but enforces it. I added some more explicit comments about what I had in mind. > > - Work to separate out the task scheduler forwarding code a bit more. > PostTask > > is the main example where you're adding a lot of complicated code into an > > already long and complicated function. Can this be separated out more? > > I could but a lot of the setup is the same, I guess I could forward the > remainder of PostTask (after the common chunk) to separate helpers, WDYT? This is what I had in mind, yes. > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > > File base/threading/sequenced_worker_pool.cc (right): > > > > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > > base/threading/sequenced_worker_pool.cc:519: /* Members used for the > > experimental redirection to TaskScheduler. */ > > Can this be a normal // comment? > > Will do.
https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:60: // because the first task was posted and the state is still NONE_ACTIVE. This should have a reference to a bug to remove it. https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:525: // A map of SequenceToken IDs to TaskScheduler TaskRunners used to redirect This should have a reference to a bug to remove it. https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:529: // A dummy TaskRunner obtained from TaskScheduler with the same TaskTraits as This should have a reference to a bug to remove it. https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:701: // Confirm that the TaskScheduler's shutdown behaviors use the same This should have a reference to a bug to remove it. https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:854: if (g_all_pools_state != AllPoolsState::WORKER_CREATED) This should have a reference to a bug to remove it. https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:1300: if (g_all_pools_state != AllPoolsState::WORKER_CREATED) { This should have a reference to a bug to remove it. https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.h:175: // Invoke this once on the main thread of a process, before any other threads This should have a reference to a bug to remove it. https://codereview.chromium.org/2241703002/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2241703002/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:457: const auto sequenced_worker_pool_param = This should have a reference to a bug to remove it.
Thanks Brett, done, PTAnL. Splitting PostTask() forced an extra AutoUnlock() scope but I still like it better, let me know what you think. https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:60: // because the first task was posted and the state is still NONE_ACTIVE. On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > This should have a reference to a bug to remove it. Done. https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:519: /* Members used for the experimental redirection to TaskScheduler. */ On 2016/08/12 17:44:39, brettw (ping on IM after 24h) wrote: > Can this be a normal // comment? Sure, a /* */ comment felt more like a split from the rest to me but I guess a // comment surrounded by empty lines works just as well. https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:525: // A map of SequenceToken IDs to TaskScheduler TaskRunners used to redirect On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > This should have a reference to a bug to remove it. Done. https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:529: // A dummy TaskRunner obtained from TaskScheduler with the same TaskTraits as On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > This should have a reference to a bug to remove it. Done. https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:701: // Confirm that the TaskScheduler's shutdown behaviors use the same On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > This should have a reference to a bug to remove it. Doesn't |g_all_pools_state| cover this? https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:854: if (g_all_pools_state != AllPoolsState::WORKER_CREATED) On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > This should have a reference to a bug to remove it. Doesn't |g_all_pools_state| cover this? https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:1300: if (g_all_pools_state != AllPoolsState::WORKER_CREATED) { On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > This should have a reference to a bug to remove it. Doesn't |g_all_pools_state| cover this? https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.h:175: // Invoke this once on the main thread of a process, before any other threads On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > This should have a reference to a bug to remove it. Done. https://codereview.chromium.org/2241703002/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2241703002/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:457: const auto sequenced_worker_pool_param = On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > This should have a reference to a bug to remove it. Done.
On 2016/08/12 23:27:46, gab (OOO Aug 15 - Sep 1) wrote: > Thanks Brett, done, PTAnL. FYI, despite my OOO status, I will be handling this CL in transit today in the hopes of unblocking the TaskScheduler team before heading on vacation. Thanks for your time, Gab > > Splitting PostTask() forced an extra AutoUnlock() scope but I still like it > better, let me know what you think. > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > File base/threading/sequenced_worker_pool.cc (right): > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > base/threading/sequenced_worker_pool.cc:60: // because the first task was posted > and the state is still NONE_ACTIVE. > On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > > This should have a reference to a bug to remove it. > > Done. > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > base/threading/sequenced_worker_pool.cc:519: /* Members used for the > experimental redirection to TaskScheduler. */ > On 2016/08/12 17:44:39, brettw (ping on IM after 24h) wrote: > > Can this be a normal // comment? > > Sure, a /* */ comment felt more like a split from the rest to me but I guess a > // comment surrounded by empty lines works just as well. > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > base/threading/sequenced_worker_pool.cc:525: // A map of SequenceToken IDs to > TaskScheduler TaskRunners used to redirect > On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > > This should have a reference to a bug to remove it. > > Done. > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > base/threading/sequenced_worker_pool.cc:529: // A dummy TaskRunner obtained from > TaskScheduler with the same TaskTraits as > On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > > This should have a reference to a bug to remove it. > > Done. > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > base/threading/sequenced_worker_pool.cc:701: // Confirm that the TaskScheduler's > shutdown behaviors use the same > On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > > This should have a reference to a bug to remove it. > > Doesn't |g_all_pools_state| cover this? > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > base/threading/sequenced_worker_pool.cc:854: if (g_all_pools_state != > AllPoolsState::WORKER_CREATED) > On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > > This should have a reference to a bug to remove it. > > Doesn't |g_all_pools_state| cover this? > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > base/threading/sequenced_worker_pool.cc:1300: if (g_all_pools_state != > AllPoolsState::WORKER_CREATED) { > On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > > This should have a reference to a bug to remove it. > > Doesn't |g_all_pools_state| cover this? > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > File base/threading/sequenced_worker_pool.h (right): > > https://codereview.chromium.org/2241703002/diff/40001/base/threading/sequence... > base/threading/sequenced_worker_pool.h:175: // Invoke this once on the main > thread of a process, before any other threads > On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > > This should have a reference to a bug to remove it. > > Done. > > https://codereview.chromium.org/2241703002/diff/40001/chrome/browser/chrome_b... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/2241703002/diff/40001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.cc:457: const auto > sequenced_worker_pool_param = > On 2016/08/12 19:30:00, brettw (ping on IM after 24h) wrote: > > This should have a reference to a bug to remove it. > > Done.
https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:800: AutoUnlock unlock(lock_); Sorry to add another pass here. I'm really reluctant to add another set of locking for every task we post to the worker pool today. It seems like this is a common operation now, nobody uses the new codepath yet, and lock contention is bad. The structure you have here is definitely more clear than the alternatives, but I think it's worth compromising a bit to avoid the lock. Can we keep this in the PostTask function like we did before (outside the lock) and only run it when we're in the old mode?
Approach LG, wondering about ordering: https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:564: DCHECK_NE(AllPoolsState::REDIRECTED_TO_TASK_SCHEDULER, g_all_pools_state); Run is happening before RedirectSequencedWorkerPoolsToTaskSchedulerForProcess? https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:774: // however a few pools that use only one thread and therefore can nit: "few pools" extra space https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:775: // currently legitimatelly assuming thread affinity despite using "legitimately assume" https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:800: AutoUnlock unlock(lock_); hm, this will relock the lock when you leave the block tho, adding contention. I'd prefer you put the AutoLock into something (a unique_ptr would work but require malloc, a base::Optional would also work and not require malloc), and move that thing into the Post method here so it can release it early. https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:921: DCHECK_NE(AllPoolsState::REDIRECTED_TO_TASK_SCHEDULER, g_all_pools_state); Why are some of these != REDIRECTED instead of == POOL? We'd make threads before setting the mode?
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Thanks, addressed/replied, PTAnL. https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:564: DCHECK_NE(AllPoolsState::REDIRECTED_TO_TASK_SCHEDULER, g_all_pools_state); On 2016/08/15 21:02:52, danakj wrote: > Run is happening before RedirectSequencedWorkerPoolsToTaskSchedulerForProcess? No, this documents that Run() should never happen in the redirected world. g_all_pools_state is used to enforce we either transition to the redirected state or the regular workers state but that we never call redirect after a Worker was started. It is also used to document (like here) which methods are exclusive to a given mode. https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:774: // however a few pools that use only one thread and therefore can On 2016/08/15 21:02:52, danakj wrote: > nit: "few pools" extra space Done. https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:775: // currently legitimatelly assuming thread affinity despite using On 2016/08/15 21:02:52, danakj wrote: > "legitimately assume" Done. https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:800: AutoUnlock unlock(lock_); On 2016/08/15 21:02:21, brettw (ping on IM after 24h) wrote: > Sorry to add another pass here. I'm really reluctant to add another set of > locking for every task we post to the worker pool today. It seems like this is a > common operation now, nobody uses the new codepath yet, and lock contention is > bad. > > The structure you have here is definitely more clear than the alternatives, but > I think it's worth compromising a bit to avoid the lock. Can we keep this in the > PostTask function like we did before (outside the lock) and only run it when > we're in the old mode? Re-inlined PostTaskToPool() -- as without this section it doesn't do much and requires having it returning int and documenting that it's the same as PrepareToStartAdditionalThreadIfHelpful() which just makes it cumbersome IMO. https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:800: AutoUnlock unlock(lock_); On 2016/08/15 21:02:52, danakj wrote: > hm, this will relock the lock when you leave the block tho, adding contention. > I'd prefer you put the AutoLock into something (a unique_ptr would work but > require malloc, a base::Optional would also work and not require malloc), and > move that thing into the Post method here so it can release it early. N/A after re-inlining PostTaskToPool per Brett's comment. https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:921: DCHECK_NE(AllPoolsState::REDIRECTED_TO_TASK_SCHEDULER, g_all_pools_state); On 2016/08/15 21:02:52, danakj wrote: > Why are some of these != REDIRECTED instead of == POOL? We'd make threads before > setting the mode? We never explicitly set POOL (a.k.a. WORKER_CREATED) from a static call. WORKER_CREATED is set when a worker is created. For that mode it's valid for the exclusive methods to run either before the mode is determined or after, it doesn't matter, as long as the redirection was explicitly asked for. It's true that in this case on ThreadLoop() it should always already be determined but I opted to document as == REDIRECTED and != REDIRECTED everywhere which is really the split I'm trying to highlight.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM % brettw https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:921: DCHECK_NE(AllPoolsState::REDIRECTED_TO_TASK_SCHEDULER, g_all_pools_state); On 2016/08/15 21:32:44, gab (OOO Aug 15 - Sep 1) wrote: > On 2016/08/15 21:02:52, danakj wrote: > > Why are some of these != REDIRECTED instead of == POOL? We'd make threads > before > > setting the mode? > > We never explicitly set POOL (a.k.a. WORKER_CREATED) from a static call. > WORKER_CREATED is set when a worker is created. For that mode it's valid for the > exclusive methods to run either before the mode is determined or after, it > doesn't matter, as long as the redirection was explicitly asked for. > > It's true that in this case on ThreadLoop() it should always already be > determined but I opted to document as == REDIRECTED and != REDIRECTED everywhere > which is really the split I'm trying to highlight. Hm ok. I would personally prefer == checks whenever it's possible since it gives the reader more information. This is a bit ambiguous it could mean it can happen earlier too. But you're also disappearing and I don't feel like strongly enough to block this or anything. https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:673: (you added whitespace here as a result)
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) ========== to ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. FYI, the experiment will not actually work until https://codereview.chromium.org/2237643002/ or a variant of it lands but this is a stepping stone to getting there and the CLs themselves can land in any order. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) ==========
Description was changed from ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. FYI, the experiment will not actually work until https://codereview.chromium.org/2237643002/ or a variant of it lands but this is a stepping stone to getting there and the CLs themselves can land in any order. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) ========== to ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. FYI, the experiment will not actually work until https://codereview.chromium.org/2237643002/ or a variant of it lands but this is a stepping stone to getting there and the CLs themselves can land in any order. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) ==========
Description was changed from ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. FYI, the experiment will not actually work until https://codereview.chromium.org/2237643002/ or a variant of it lands but this is a stepping stone to getting there and the CLs themselves can land in any order. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) ========== to ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. FYI, the experiment will not actually work until https://codereview.chromium.org/2237643002/ or a variant of it lands but this is a stepping stone to getting there and the CLs themselves can land in any order. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) ==========
Description was changed from ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. FYI, the experiment will not actually work until https://codereview.chromium.org/2237643002/ or a variant of it lands but this is a stepping stone to getting there and the CLs themselves can land in any order. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) ========== to ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. FYI, the experiment (and TEST= below) will not actually work until https://codereview.chromium.org/2237643002/ or a variant of it lands but this is a stepping stone to getting there and the CLs themselves can land in any order. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) ==========
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...
Just back online for an instant now, will try to land this, amended CL description to indicate that https://codereview.chromium.org/2237643002/ is still required for the experiment to actually work (but it's not live yet so that's fine) https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2241703002/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:921: DCHECK_NE(AllPoolsState::REDIRECTED_TO_TASK_SCHEDULER, g_all_pools_state); On 2016/08/15 22:05:49, danakj wrote: > On 2016/08/15 21:32:44, gab (OOO Aug 15 - Sep 1) wrote: > > On 2016/08/15 21:02:52, danakj wrote: > > > Why are some of these != REDIRECTED instead of == POOL? We'd make threads > > before > > > setting the mode? > > > > We never explicitly set POOL (a.k.a. WORKER_CREATED) from a static call. > > WORKER_CREATED is set when a worker is created. For that mode it's valid for > the > > exclusive methods to run either before the mode is determined or after, it > > doesn't matter, as long as the redirection was explicitly asked for. > > > > It's true that in this case on ThreadLoop() it should always already be > > determined but I opted to document as == REDIRECTED and != REDIRECTED > everywhere > > which is really the split I'm trying to highlight. > > Hm ok. I would personally prefer == checks whenever it's possible since it gives > the reader more information. This is a bit ambiguous it could mean it can happen > earlier too. But you're also disappearing and I don't feel like strongly enough > to block this or anything. Unless the first Worker is created while only the main thread is around, checking for == WORKER_CREATED is racy from pre-existing threads (this is why the API for the redirection is strict on it being called before any other threads are created). This is why no logic is based on it being set and also why I think it's more correct to not use it in DCHECKs either (although == would be fine in logic that only runs from the worker threads themselves -- I can do that when I'm back if you strongly prefer).
Message was sent while issue was closed.
Description was changed from ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. FYI, the experiment (and TEST= below) will not actually work until https://codereview.chromium.org/2237643002/ or a variant of it lands but this is a stepping stone to getting there and the CLs themselves can land in any order. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) ========== to ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. FYI, the experiment (and TEST= below) will not actually work until https://codereview.chromium.org/2237643002/ or a variant of it lands but this is a stepping stone to getting there and the CLs themselves can land in any order. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. FYI, the experiment (and TEST= below) will not actually work until https://codereview.chromium.org/2237643002/ or a variant of it lands but this is a stepping stone to getting there and the CLs themselves can land in any order. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) ========== to ========== Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler. The experiment is controlled by a static call for the entire process which must be made before any SequencedWorkerPool instance is posted a task (i.e. before it creates a Worker). The experiment being at run-time, sequenced_worker_pool.cc is now divided between the two worlds. DCHECKs were added to associate as many methods as possible to the world they belong to. FYI, the experiment (and TEST= below) will not actually work until https://codereview.chromium.org/2237643002/ or a variant of it lands but this is a stepping stone to getting there and the CLs themselves can land in any order. BUG=622400 TEST= A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing B) "out\Debug\chrome --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" runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-) Committed: https://crrev.com/5a65a6844ec5348feedb041cad8721ab79715395 Cr-Commit-Position: refs/heads/master@{#413346} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5a65a6844ec5348feedb041cad8721ab79715395 Cr-Commit-Position: refs/heads/master@{#413346}
Message was sent while issue was closed.
fdoray@chromium.org changed reviewers: + fdoray@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:748: const SequencedTask& sequenced) { This method doesn't honor delays :( https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:775: .WithShutdownBehavior(task_shutdown_behavior); SequencedWorkerPool allows tasks with different shutdown behaviors to be posted to the same sequence. TaskScheduler currently doesn't support that. Do we want to DCHECK that this doesn't happen for tasks forwarded to the TaskScheduler? https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:786: : ExecutionMode::SEQUENCED; Should we DCHECK_LE(sequenced_task_runner_map_.size(), 1)? Otherwise, tasks posted to different sequences on the same single-threaded SequencedWorkerPool may run on different threads. https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:815: ExecutionMode::PARALLEL); Should we have a special case for single-threaded SequencedWorkerPools here?
Message was sent while issue was closed.
Thanks for the nice post-commit catches :-). Looks like you even followed up with all of them in my absence on https://codereview.chromium.org/2285633003/ so yay! https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:673: On 2016/08/15 22:05:49, danakj wrote: > (you added whitespace here as a result) True, but I kind of think it's cleaner that way as the statement applies to the whole block not just the following one? https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:748: const SequencedTask& sequenced) { On 2016/08/25 15:14:01, fdoray wrote: > This method doesn't honor delays :( Oops, looks like you're addressing that in https://codereview.chromium.org/2285633003/? Thanks! https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:775: .WithShutdownBehavior(task_shutdown_behavior); On 2016/08/25 15:14:01, fdoray wrote: > SequencedWorkerPool allows tasks with different shutdown behaviors to be posted > to the same sequence. TaskScheduler currently doesn't support that. Do we want > to DCHECK that this doesn't happen for tasks forwarded to the TaskScheduler? Yes, let's do that (seems like that's what you're doing in https://codereview.chromium.org/2285633003/ :-)) https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:786: : ExecutionMode::SEQUENCED; On 2016/08/25 15:14:01, fdoray wrote: > Should we DCHECK_LE(sequenced_task_runner_map_.size(), 1)? Otherwise, tasks > posted to different sequences on the same single-threaded SequencedWorkerPool > may run on different threads. Good point, I think this is the case in practice so that sounds fine to me. https://codereview.chromium.org/2241703002/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:815: ExecutionMode::PARALLEL); On 2016/08/25 15:14:01, fdoray wrote: > Should we have a special case for single-threaded SequencedWorkerPools here? Yes, looks like you got this too :-) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
