Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2077413009:
Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (Closed)
|
Created:
4 years, 6 months ago by gab Modified:
4 years, 5 months ago Reviewers:
Primiano Tucci (use gerrit), sky, Ryan Sleevi, gavinp, jam, Scott Byer, danakj, scheib, eroman, sdefresne, Paweł Hajdan Jr., brettw CC:
cbentzel+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, gavinp+disk_chromium.org, kalyank, kinuko+cache_chromium.org, ortuno+watch_chromium.org, sadrul, scheib+watch_chromium.org, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@a2_hook Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment.
(ref. http://crbug.com/622400 for experiment details)
The mapping should be as follows:
- TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to
a user action.
- TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes)
but can be re-ordered behind USER_BLOCKING work.
- TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere
with foreground work.
Since each pool can only be assigned a single priority (until we do the full migration and each
TaskRunner extracted from it can have its own priority), the highest priority among all tasks
running in that pool should be picked if the pool runs multiple types of tasks.
Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority
but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler
will likely soon replace those use cases.
BUG=553459, 622400
Committed: https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56
Cr-Commit-Position: refs/heads/master@{#406481}
Patch Set 1 #
Total comments: 10
Patch Set 2 : review:jam #
Total comments: 6
Patch Set 3 : review:scheib/eroman 2x USER_BLOCKING->USER_VISIBLE + format #Patch Set 4 : fix compile (+child_process_host_unittest.cc) #
Total comments: 2
Patch Set 5 : add dhcp_proxy_script_adapter_fetcher_win_unittest.cc => USER_VISIBLE #Patch Set 6 : +ios/, +net\disk_cache\blockfile\file_posix.cc => USER_BLOCKING, tools\gn => USER_VISIBLE #Patch Set 7 : +tools/gn => USER_VISIBLE #Patch Set 8 : BlockingPool => USER_VISIBLE #Patch Set 9 : fix ios compile #Patch Set 10 : merge up to r404204 #Patch Set 11 : re-add old constructor as deprecated for few remaining callers #Patch Set 12 : tweak comment #Messages
Total messages: 72 (34 generated)
Description was changed from
==========
Add TaskPriority as a parameter to SequencedWorkerPool in preparation for
TaskScheduler experiment.
BUG=553459
==========
to
==========
Add TaskPriority as a parameter to SequencedWorkerPool in preparation for
TaskScheduler experiment.
The mapping should be as follows:
- TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking
path to responding to
a user action.
- TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e.
have visible outcomes)
but can be re-ordered behind USER_BLOCKING
work.
- TaskPriority::BACKGROUND : the pool runs non-critical background tasks that
should not interfere
with foreground work.
Since each pool can only be assigned a single priority (until we do the full
migration and each
TaskRunner extracted from it can have its own priority), the highest priority
among all tasks
running in that pool should be picked if the pool runs both types of tasks.
Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at
background OS priority
but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's
tasks. A TestTaskScheduler
will likely soon replace those use cases.
Owners to stamp each change based on the above:
sky@chromium.org:
- ash/sysui/sysui_application.cc
phajdan.jr@chromium.org:
- base/test/sequenced_worker_pool_owner.cc
nduca@chromium.org:
- base/trace_event/memory_dump_manager_unittest.cc
gene@chromium.org:
- chrome/service/service_process.cc
rsleevi@chromium.org:
- components/webcrypto/webcrypto_impl.cc
jam@chromium.org:
- content/browser/browser_thread_impl.cc
- services/shell/standalone/context.cc
scheib@chromium.org:
- device/bluetooth/bluetooth_task_manager_win.cc
gavinp@chromium.org:
- net/disk_cache/simple/simple_backend_impl.cc
- net/proxy/dhcp_proxy_script_fetcher_win.cc
- net/ssl/ssl_platform_key_task_runner.cc
Main API change, danakj@chromium.org:
- base/threading/sequenced_worker_pool.cc
- base/threading/sequenced_worker_pool.h
BUG=553459
==========
Description was changed from
==========
Add TaskPriority as a parameter to SequencedWorkerPool in preparation for
TaskScheduler experiment.
The mapping should be as follows:
- TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking
path to responding to
a user action.
- TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e.
have visible outcomes)
but can be re-ordered behind USER_BLOCKING
work.
- TaskPriority::BACKGROUND : the pool runs non-critical background tasks that
should not interfere
with foreground work.
Since each pool can only be assigned a single priority (until we do the full
migration and each
TaskRunner extracted from it can have its own priority), the highest priority
among all tasks
running in that pool should be picked if the pool runs both types of tasks.
Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at
background OS priority
but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's
tasks. A TestTaskScheduler
will likely soon replace those use cases.
Owners to stamp each change based on the above:
sky@chromium.org:
- ash/sysui/sysui_application.cc
phajdan.jr@chromium.org:
- base/test/sequenced_worker_pool_owner.cc
nduca@chromium.org:
- base/trace_event/memory_dump_manager_unittest.cc
gene@chromium.org:
- chrome/service/service_process.cc
rsleevi@chromium.org:
- components/webcrypto/webcrypto_impl.cc
jam@chromium.org:
- content/browser/browser_thread_impl.cc
- services/shell/standalone/context.cc
scheib@chromium.org:
- device/bluetooth/bluetooth_task_manager_win.cc
gavinp@chromium.org:
- net/disk_cache/simple/simple_backend_impl.cc
- net/proxy/dhcp_proxy_script_fetcher_win.cc
- net/ssl/ssl_platform_key_task_runner.cc
Main API change, danakj@chromium.org:
- base/threading/sequenced_worker_pool.cc
- base/threading/sequenced_worker_pool.h
BUG=553459
==========
to
==========
Add TaskPriority as a parameter to SequencedWorkerPool in preparation for
TaskScheduler experiment.
The mapping should be as follows:
- TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking
path to responding to
a user action.
- TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e.
have visible outcomes)
but can be re-ordered behind USER_BLOCKING
work.
- TaskPriority::BACKGROUND : the pool runs non-critical background tasks that
should not interfere
with foreground work.
Since each pool can only be assigned a single priority (until we do the full
migration and each
TaskRunner extracted from it can have its own priority), the highest priority
among all tasks
running in that pool should be picked if the pool runs both types of tasks.
Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at
background OS priority
but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's
tasks. A TestTaskScheduler
will likely soon replace those use cases.
Owners to stamp each change based on the above:
sky@chromium.org:
- ash/sysui/sysui_application.cc
phajdan.jr@chromium.org:
- base/test/sequenced_worker_pool_owner.cc
nduca@chromium.org:
- base/trace_event/memory_dump_manager_unittest.cc
gene@chromium.org:
- chrome/service/service_process.cc
rsleevi@chromium.org:
- components/webcrypto/webcrypto_impl.cc
jam@chromium.org:
- content/browser/browser_thread_impl.cc
- services/shell/standalone/context.cc
scheib@chromium.org:
- device/bluetooth/bluetooth_task_manager_win.cc
gavinp@chromium.org:
- net/disk_cache/simple/simple_backend_impl.cc
- net/proxy/dhcp_proxy_script_fetcher_win.cc
- net/ssl/ssl_platform_key_task_runner.cc
Main API change, danakj@chromium.org:
- base/threading/sequenced_worker_pool.cc
- base/threading/sequenced_worker_pool.h
BUG=553459, 622400
==========
Description was changed from
==========
Add TaskPriority as a parameter to SequencedWorkerPool in preparation for
TaskScheduler experiment.
The mapping should be as follows:
- TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking
path to responding to
a user action.
- TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e.
have visible outcomes)
but can be re-ordered behind USER_BLOCKING
work.
- TaskPriority::BACKGROUND : the pool runs non-critical background tasks that
should not interfere
with foreground work.
Since each pool can only be assigned a single priority (until we do the full
migration and each
TaskRunner extracted from it can have its own priority), the highest priority
among all tasks
running in that pool should be picked if the pool runs both types of tasks.
Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at
background OS priority
but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's
tasks. A TestTaskScheduler
will likely soon replace those use cases.
Owners to stamp each change based on the above:
sky@chromium.org:
- ash/sysui/sysui_application.cc
phajdan.jr@chromium.org:
- base/test/sequenced_worker_pool_owner.cc
nduca@chromium.org:
- base/trace_event/memory_dump_manager_unittest.cc
gene@chromium.org:
- chrome/service/service_process.cc
rsleevi@chromium.org:
- components/webcrypto/webcrypto_impl.cc
jam@chromium.org:
- content/browser/browser_thread_impl.cc
- services/shell/standalone/context.cc
scheib@chromium.org:
- device/bluetooth/bluetooth_task_manager_win.cc
gavinp@chromium.org:
- net/disk_cache/simple/simple_backend_impl.cc
- net/proxy/dhcp_proxy_script_fetcher_win.cc
- net/ssl/ssl_platform_key_task_runner.cc
Main API change, danakj@chromium.org:
- base/threading/sequenced_worker_pool.cc
- base/threading/sequenced_worker_pool.h
BUG=553459, 622400
==========
to
==========
Add TaskPriority as a parameter to SequencedWorkerPool in preparation for
TaskScheduler experiment.
(ref. http://crbug.com/622400 for experiment details)
The mapping should be as follows:
- TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking
path to responding to
a user action.
- TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e.
have visible outcomes)
but can be re-ordered behind USER_BLOCKING
work.
- TaskPriority::BACKGROUND : the pool runs non-critical background tasks that
should not interfere
with foreground work.
Since each pool can only be assigned a single priority (until we do the full
migration and each
TaskRunner extracted from it can have its own priority), the highest priority
among all tasks
running in that pool should be picked if the pool runs both types of tasks.
Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at
background OS priority
but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's
tasks. A TestTaskScheduler
will likely soon replace those use cases.
Owners to stamp each change based on the above:
sky@chromium.org:
- ash/sysui/sysui_application.cc
phajdan.jr@chromium.org:
- base/test/sequenced_worker_pool_owner.cc
nduca@chromium.org:
- base/trace_event/memory_dump_manager_unittest.cc
gene@chromium.org:
- chrome/service/service_process.cc
rsleevi@chromium.org:
- components/webcrypto/webcrypto_impl.cc
jam@chromium.org:
- content/browser/browser_thread_impl.cc
- services/shell/standalone/context.cc
scheib@chromium.org:
- device/bluetooth/bluetooth_task_manager_win.cc
gavinp@chromium.org:
- net/disk_cache/simple/simple_backend_impl.cc
- net/proxy/dhcp_proxy_script_fetcher_win.cc
- net/ssl/ssl_platform_key_task_runner.cc
Main API change, danakj@chromium.org:
- base/threading/sequenced_worker_pool.cc
- base/threading/sequenced_worker_pool.h
BUG=553459, 622400
==========
Folks PTAL (there are many of you but you each have a very specific role, see CL description :-)), thanks!
rsleevi@chromium.org changed reviewers: + eroman@chromium.org
Eric is a better reviewer for the WebCrypto & Proxy changes. I'm a better reviewer for the SSLPlatformKey changes - and those LGTM now, but I'll reiterate that it should not be moved to a TaskScheduler.
LGTM
On 2016/06/22 20:00:35, Ryan Sleevi wrote: > Eric is a better reviewer for the WebCrypto & Proxy changes. > I'm a better reviewer for the SSLPlatformKey changes - and those LGTM now, but > I'll reiterate that it should not be moved to a TaskScheduler. The TaskScheduler has a notion of SINGLE_THREADED work so it should be safe to move it. It will be equivalent to a SequencedWorkerPool with 1 thread.
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs both types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Owners to stamp each change based on the above: sky@chromium.org: - ash/sysui/sysui_application.cc phajdan.jr@chromium.org: - base/test/sequenced_worker_pool_owner.cc nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc rsleevi@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc scheib@chromium.org: - device/bluetooth/bluetooth_task_manager_win.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs both types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Owners to stamp each change based on the above: sky@chromium.org: - ash/sysui/sysui_application.cc phajdan.jr@chromium.org: - base/test/sequenced_worker_pool_owner.cc nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc rsleevi@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc scheib@chromium.org: - device/bluetooth/bluetooth_task_manager_win.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ==========
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs both types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Owners to stamp each change based on the above: sky@chromium.org: - ash/sysui/sysui_application.cc phajdan.jr@chromium.org: - base/test/sequenced_worker_pool_owner.cc nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc rsleevi@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc scheib@chromium.org: - device/bluetooth/bluetooth_task_manager_win.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs both types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Owners to stamp each change based on the above: sky@chromium.org: - ash/sysui/sysui_application.cc phajdan.jr@chromium.org: - base/test/sequenced_worker_pool_owner.cc nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc eroman@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc scheib@chromium.org: - device/bluetooth/bluetooth_task_manager_win.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ==========
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs both types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Owners to stamp each change based on the above: sky@chromium.org: - ash/sysui/sysui_application.cc phajdan.jr@chromium.org: - base/test/sequenced_worker_pool_owner.cc nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc eroman@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc scheib@chromium.org: - device/bluetooth/bluetooth_task_manager_win.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs both types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Owners to stamp each change based on the above: sky@chromium.org: - ash/sysui/sysui_application.cc phajdan.jr@chromium.org: - base/test/sequenced_worker_pool_owner.cc nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc eroman@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc scheib@chromium.org: - device/bluetooth/bluetooth_task_manager_win.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ==========
On 2016/06/22 20:06:21, gab wrote: > On 2016/06/22 20:00:35, Ryan Sleevi wrote: > > Eric is a better reviewer for the WebCrypto & Proxy changes. > > I'm a better reviewer for the SSLPlatformKey changes - and those LGTM now, but > > I'll reiterate that it should not be moved to a TaskScheduler. > > The TaskScheduler has a notion of SINGLE_THREADED work so it should be safe to > move it. It will be equivalent to a SequencedWorkerPool with 1 thread. I can follow-up with you offline, but the threading requirements of NSS are complex enough that you should not be changing how SSLPlatformKey behaves.
Two notes. I think we need to be very careful in how we pick the priority. nit: i would move the reviewer split into a message instead of in the cl description. the latter puts it in git history, which seems like it just adds noise. https://codereview.chromium.org/2077413009/diff/1/chrome/service/service_proc... File chrome/service/service_process.cc (right): https://codereview.chromium.org/2077413009/diff/1/chrome/service/service_proc... chrome/service/service_process.cc:156: 3, "ServiceBlocking", base::TaskPriority::BACKGROUND); users will notice if this takes arbitrary long time, i.e. printing jobs won't print https://codereview.chromium.org/2077413009/diff/1/content/browser/browser_thr... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2077413009/diff/1/content/browser/browser_thr... content/browser/browser_thread_impl.cc:106: base::TaskPriority::BACKGROUND)) { the blocking pool is for anything that needs to write to disk. some of those tasks may be background, but not all. how did you arrive at this priority? https://codereview.chromium.org/2077413009/diff/1/services/shell/standalone/c... File services/shell/standalone/context.cc (right): https://codereview.chromium.org/2077413009/diff/1/services/shell/standalone/c... services/shell/standalone/context.cc:141: kMaxBlockingPoolThreads, "blocking_pool", base::TaskPriority::BACKGROUND); similarly, i don't think this should be priority. when the mojo shell is reading manifests (note, this isn't in production at the moment), it's blocked on loading apps' manifest.
On 2016/06/22 20:15:50, Ryan Sleevi wrote: > On 2016/06/22 20:06:21, gab wrote: > > On 2016/06/22 20:00:35, Ryan Sleevi wrote: > > > Eric is a better reviewer for the WebCrypto & Proxy changes. > > > I'm a better reviewer for the SSLPlatformKey changes - and those LGTM now, > but > > > I'll reiterate that it should not be moved to a TaskScheduler. > > > > The TaskScheduler has a notion of SINGLE_THREADED work so it should be safe to > > move it. It will be equivalent to a SequencedWorkerPool with 1 thread. > > I can follow-up with you offline, but the threading requirements of NSS are > complex enough that you should not be changing how SSLPlatformKey behaves. It will still run on a single-thread, just a different one (which might interleave other work), isn't that okay?
On 2016/06/22 21:00:15, gab wrote: > It will still run on a single-thread, just a different one (which might > interleave other work), isn't that okay? 1) It needs to be the same thread throughout (NSS & CSSM sets state and has affinity to the specific thread) 2) If those threads interleave other work, it is no longer possible for me to reason whether or not the call sequencing will be violated. That's why an explicit, intentional dedicated thread was used, and which only processes private key operations. It sounds like this really "Not LGTM" - because the scope is greater than what we'd previously discussed and the concerns raised. The threading issues are complex enough - and the time spent debugging and fixing has been significant enough - that I'm explicitly not comfortable making this change at this time for the SSL thread.
On 2016/06/22 20:17:37, jam wrote: > Two notes. I think we need to be very careful in how we pick the priority. > > nit: i would move the reviewer split into a message instead of in the cl > description. the latter puts it in git history, which seems like it just adds > noise. Except that messages get lost in the stream of replies, does anybody really care about a clean git history per commit? I'm happy to strip that part before landing if so, but think it helps the review in the mean time. https://codereview.chromium.org/2077413009/diff/1/chrome/service/service_proc... File chrome/service/service_process.cc (right): https://codereview.chromium.org/2077413009/diff/1/chrome/service/service_proc... chrome/service/service_process.cc:156: 3, "ServiceBlocking", base::TaskPriority::BACKGROUND); On 2016/06/22 20:17:37, jam wrote: > users will notice if this takes arbitrary long time, i.e. printing jobs won't > print Okay changing to USER_VISIBLE to keep it as foreground pooled work (and either way service processes will not be running TaskScheduler right away). https://codereview.chromium.org/2077413009/diff/1/content/browser/browser_thr... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2077413009/diff/1/content/browser/browser_thr... content/browser/browser_thread_impl.cc:106: base::TaskPriority::BACKGROUND)) { On 2016/06/22 20:17:37, jam wrote: > the blocking pool is for anything that needs to write to disk. some of those > tasks may be background, but not all. > > how did you arrive at this priority? IMO anything that goes in the BlockingPool in the browser shouldn't expect any kind of throughput. The critical disk access is on the CachePool, the BlockingPool should only be used for background work. Or are you aware of different types of work going into the BlockingPool? https://codereview.chromium.org/2077413009/diff/1/services/shell/standalone/c... File services/shell/standalone/context.cc (right): https://codereview.chromium.org/2077413009/diff/1/services/shell/standalone/c... services/shell/standalone/context.cc:141: kMaxBlockingPoolThreads, "blocking_pool", base::TaskPriority::BACKGROUND); On 2016/06/22 20:17:37, jam wrote: > similarly, i don't think this should be priority. when the mojo shell is reading > manifests (note, this isn't in production at the moment), it's blocked on > loading apps' manifest. Okay then everything but reading manifests should be BACKGROUND? And by virtue of the comment in the CL description we have to take the maximum priority of all potential tasks for now (until they are split in multiple task runners instead of associated to a single pool). So USER_VISIBLE?
On 2016/06/22 21:06:45, Ryan Sleevi wrote: > On 2016/06/22 21:00:15, gab wrote: > > It will still run on a single-thread, just a different one (which might > > interleave other work), isn't that okay? > > 1) It needs to be the same thread throughout (NSS & CSSM sets state and has > affinity to the specific thread) It will be the same throughout, that's the guarantee of ExecutionMode::SINGLE_THREADED. > 2) If those threads interleave other work, it is no longer possible for me to > reason whether or not the call sequencing will be violated. That's why an > explicit, intentional dedicated thread was used, and which only processes > private key operations. It can't be interleaved with unrelated work? If that's really the case they it sounds like you need your own thread. Curious why you're using SequencedWorkerPool though? Is it only for the ShutdownBehavior semantics? Otherwise a plain Thread gives you the same thing, doesn't it? We're looking to get rid of SequencedWorkerPool so we'll need to find a solution here. > > It sounds like this really "Not LGTM" - because the scope is greater than what > we'd previously discussed and the concerns raised. The threading issues are > complex enough - and the time spent debugging and fixing has been significant > enough - that I'm explicitly not comfortable making this change at this time for > the SSL thread.
On 2016/06/22 21:11:48, gab (OOO back Monday) wrote: > It can't be interleaved with unrelated work? The PrivateKey operations can end up blocking on other tasks posted to other threads, and if those were to end up posting to the same TaskRunner, we'd have a deadlock. Reasoning about the complexities of that codepath when interacting with libraries we don't control and whose contracts are not well documented (Apple), not well followed (Microsoft), or not well contained (NSS) is what I'm suggesting is a large request that we don't have the bandwidth to do at present, and which have spent Eng-Months dealing with in the past, so we're more conservative now (I think of a small change in GPU task posting that broken TLS as an example of this, for reasons of shared state interrelationship) > If that's really the case they it > sounds like you need your own thread. Curious why you're using > SequencedWorkerPool though? Is it only for the ShutdownBehavior semantics? > Otherwise a plain Thread gives you the same thing, doesn't it? We're looking to > get rid of SequencedWorkerPool so we'll need to find a solution here. A base::Thread requires shutting down, which we explicitly don't want. A base::PlatformThread doesn't provide any task sequencing facilities When this code was introduced, I went through several iterations with Darin after various assumptions of //base interacted poorly with the needs here. The SWP of 1 was chosen through working with akalin, willchan, and darin - the three people most responsible for the abstractions here. So no, a plain Thread doesn't offer the same semantics.
On 2016/06/22 21:08:56, gab (OOO back Monday) wrote: > On 2016/06/22 20:17:37, jam wrote: > > Two notes. I think we need to be very careful in how we pick the priority. > > > > nit: i would move the reviewer split into a message instead of in the cl > > description. the latter puts it in git history, which seems like it just adds > > noise. > > Except that messages get lost in the stream of replies, does anybody really care > about a clean git history per commit? I'm happy to strip that part before > landing if so, but think it helps the review in the mean time. note: again this is a nit and i leave it up to you. I brought this up because the cl description.. is supposed to be a description of the change. putting instructions for code reviews there seemed unnecessary. in all other reviews i've been on when there are multiple reviewers, the first message (or subsequent ones) called this out and it worked out fine. https://codereview.chromium.org/2077413009/diff/1/content/browser/browser_thr... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2077413009/diff/1/content/browser/browser_thr... content/browser/browser_thread_impl.cc:106: base::TaskPriority::BACKGROUND)) { On 2016/06/22 21:08:56, gab (OOO back Monday) wrote: > On 2016/06/22 20:17:37, jam wrote: > > the blocking pool is for anything that needs to write to disk. some of those > > tasks may be background, but not all. > > > > how did you arrive at this priority? > > IMO anything that goes in the BlockingPool in the browser shouldn't expect any > kind of throughput. The critical disk access is on the CachePool, the > BlockingPool should only be used for background work. > > Or are you aware of different types of work going into the BlockingPool? In general I agree. However it's not clear that this is shared by all consumers. Have you audited the 355 callers of this method? the documentation states: // Returns the thread pool used for blocking file I/O. Use this object to // perform random blocking operations such as file writes or querying the // Windows registry. It doesn't say that this is for things that "shouldn't expect any kind of throughput". If you'd like to change the behavior, that's fine, but the onus should be to first check existing callers.
LGTM, though I note that perhaps the priority can be lower for Bluetooth: https://codereview.chromium.org/2077413009/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://codereview.chromium.org/2077413009/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_task_manager_win.cc:262: base::TaskPriority::USER_BLOCKING); I'm not fully familiar with the priority levels, but this may be unnecessarily high. Some of the bluetooth actions are visible in user interface, e.g. when discovering devices. And, reading and writing values may happen as a part of user interface interaction (including via web page UI). But, I do not believe any of these APIs presume a very rapid response, e.g. expected to appear as 'instant' when reacting to a user button press instantly, or animation, etc. Perhaps USER_VISIBLE is reasonable. I don't expect this code to be exercised very much currently.
LGTM for net/proxy/* and components/webcrypto/* https://codereview.chromium.org/2077413009/diff/20001/net/proxy/dhcp_proxy_sc... File net/proxy/dhcp_proxy_script_fetcher_win.cc (right): https://codereview.chromium.org/2077413009/diff/20001/net/proxy/dhcp_proxy_sc... net/proxy/dhcp_proxy_script_fetcher_win.cc:62: new base::SequencedWorkerPool(kMaxDhcpLookupThreads, "PacDhcpLookup", BTW I don't believe this actually has any ordering requirements. Looks like it is just using the maximum threads parameter to rate limit the requests made, could be changed to ExecutionMode:PARALLEL in the future. https://codereview.chromium.org/2077413009/diff/20001/net/proxy/dhcp_proxy_sc... net/proxy/dhcp_proxy_script_fetcher_win.cc:63: base::TaskPriority::USER_BLOCKING); USER_VISIBLE should be good enough for this. As an aside, I wonder if just these 3 priority classes will be sufficiently granular to make good scheduling choices. Here for instance after the transition we could just be firing off all the tasks with ExecutionMode::PARALLEL and priority=USER_VISIBLE. That is fine, but in theory we have more information available. Some of the tasks (adapter probes) are less important than others and could be de-prioritized. But not so unimportant as to become BACKGROUND. This particular case isn't all that interesting (wasn't really optimized before), but still curious if there are plans to have more priorities or other ways to describe precedence of tasks than those 3.
base/test LGTM
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs both types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Owners to stamp each change based on the above: sky@chromium.org: - ash/sysui/sysui_application.cc phajdan.jr@chromium.org: - base/test/sequenced_worker_pool_owner.cc nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc eroman@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc scheib@chromium.org: - device/bluetooth/bluetooth_task_manager_win.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Owners to stamp each change based on the above: sky@chromium.org: - ash/sysui/sysui_application.cc phajdan.jr@chromium.org: - base/test/sequenced_worker_pool_owner.cc nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc eroman@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc scheib@chromium.org: - device/bluetooth/bluetooth_task_manager_win.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ==========
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Owners to stamp each change based on the above: sky@chromium.org: - ash/sysui/sysui_application.cc phajdan.jr@chromium.org: - base/test/sequenced_worker_pool_owner.cc nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc eroman@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc scheib@chromium.org: - device/bluetooth/bluetooth_task_manager_win.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): sky@chromium.org: - ash/sysui/sysui_application.cc nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc eroman@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ==========
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): sky@chromium.org: - ash/sysui/sysui_application.cc nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc eroman@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc eroman@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ==========
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc eroman@chromium.org: - components/webcrypto/webcrypto_impl.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc eroman@chromium.org: - components/webcrypto/webcrypto_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ==========
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc eroman@chromium.org: - components/webcrypto/webcrypto_impl.cc - net/proxy/dhcp_proxy_script_fetcher_win.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ==========
Ping reviewers: nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h Pending discussion (see replies below): rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc Done, thanks: sky@chromium.org: - ash/sysui/sysui_application.cc phajdan.jr@chromium.org: - base/test/sequenced_worker_pool_owner.cc @rsleevi: On 2016/06/22 21:28:57, Ryan Sleevi wrote: > On 2016/06/22 21:11:48, gab (OOO back Monday) wrote: > > It can't be interleaved with unrelated work? > > The PrivateKey operations can end up blocking on other tasks posted to other > threads, and if those were to end up posting to the same TaskRunner, we'd have a > deadlock. Reasoning about the complexities of that codepath when interacting > with libraries we don't control and whose contracts are not well documented > (Apple), not well followed (Microsoft), or not well contained (NSS) is what I'm > suggesting is a large request that we don't have the bandwidth to do at present, > and which have spent Eng-Months dealing with in the past, so we're more > conservative now (I think of a small change in GPU task posting that broken TLS > as an example of this, for reasons of shared state interrelationship) Got it, just to be clear we are strictly talking about SSLPlatformKeyTaskRunner right? Changes to the other pools have been lgtm'ed by eroman@ so I assume it's okay? Thanks for pointing out the generic problem of having to keep inter-dependent SingleThreadTaskRunner on separate underlying threads, we will have to deal with this in general anyways. > > > If that's really the case they it > > sounds like you need your own thread. Curious why you're using > > SequencedWorkerPool though? Is it only for the ShutdownBehavior semantics? > > Otherwise a plain Thread gives you the same thing, doesn't it? We're looking > to > > get rid of SequencedWorkerPool so we'll need to find a solution here. > > A base::Thread requires shutting down, which we explicitly don't want. > A base::PlatformThread doesn't provide any task sequencing facilities > > When this code was introduced, I went through several iterations with Darin > after various assumptions of //base interacted poorly with the needs here. The > SWP of 1 was chosen through working with akalin, willchan, and darin - the three > people most responsible for the abstractions here. > > So no, a plain Thread doesn't offer the same semantics. Got it, today there is PlatformThread::CreateNonJoinable(). Would a base::Thread:Options that allows it to be non-joinable work for you? Seems to me like it would achieve the same goal. I'm surprised by the lack of comments on ssl_platform_key_task_runner.(cc|h) explaining what seems like a very intricate design decision. I'm happy to put a CL together and work with you on this. Like I said, SequencedWorkerPool will go away and its only use case can't remain something which is inherently single-threaded. https://codereview.chromium.org/2077413009/diff/1/content/browser/browser_thr... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2077413009/diff/1/content/browser/browser_thr... content/browser/browser_thread_impl.cc:106: base::TaskPriority::BACKGROUND)) { On 2016/06/22 21:38:44, jam wrote: > On 2016/06/22 21:08:56, gab (OOO back Monday) wrote: > > On 2016/06/22 20:17:37, jam wrote: > > > the blocking pool is for anything that needs to write to disk. some of those > > > tasks may be background, but not all. > > > > > > how did you arrive at this priority? > > > > IMO anything that goes in the BlockingPool in the browser shouldn't expect any > > kind of throughput. The critical disk access is on the CachePool, the > > BlockingPool should only be used for background work. > > > > Or are you aware of different types of work going into the BlockingPool? > > In general I agree. However it's not clear that this is shared by all consumers. > Have you audited the 355 callers of this method? the documentation states: > // Returns the thread pool used for blocking file I/O. Use this object to > // perform random blocking operations such as file writes or querying the > // Windows registry. > > It doesn't say that this is for things that "shouldn't expect any kind of > throughput". If you'd like to change the behavior, that's fine, but the onus > should be to first check existing callers. I don't think it's reasonable for any caller today to use the BlockingPool for tasks that require responsiveness specifically because it documents itself as being used to "perform random blocking operations" and as such even non-blocking operations can be blocked behind other ones. As such I don't think an audit is required as that would be a major undertaking for an API which I honestly don't think should be used for the surveyed use case. https://codereview.chromium.org/2077413009/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://codereview.chromium.org/2077413009/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_task_manager_win.cc:262: base::TaskPriority::USER_BLOCKING); On 2016/06/23 00:49:30, scheib wrote: > I'm not fully familiar with the priority levels, but this may be unnecessarily > high. Some of the bluetooth actions are visible in user interface, e.g. when > discovering devices. And, reading and writing values may happen as a part of > user interface interaction (including via web page UI). But, I do not believe > any of these APIs presume a very rapid response, e.g. expected to appear as > 'instant' when reacting to a user button press instantly, or animation, etc. > > Perhaps USER_VISIBLE is reasonable. SGTM, thanks, done. > > I don't expect this code to be exercised very much currently. https://codereview.chromium.org/2077413009/diff/20001/net/proxy/dhcp_proxy_sc... File net/proxy/dhcp_proxy_script_fetcher_win.cc (right): https://codereview.chromium.org/2077413009/diff/20001/net/proxy/dhcp_proxy_sc... net/proxy/dhcp_proxy_script_fetcher_win.cc:62: new base::SequencedWorkerPool(kMaxDhcpLookupThreads, "PacDhcpLookup", On 2016/06/23 01:19:21, eroman wrote: > BTW I don't believe this actually has any ordering requirements. > > Looks like it is just using the maximum threads parameter to rate limit the > requests made, could be changed to ExecutionMode:PARALLEL in the future. Ack, will keep in mind for later, thanks! https://codereview.chromium.org/2077413009/diff/20001/net/proxy/dhcp_proxy_sc... net/proxy/dhcp_proxy_script_fetcher_win.cc:63: base::TaskPriority::USER_BLOCKING); On 2016/06/23 01:19:21, eroman wrote: > USER_VISIBLE should be good enough for this. Done. > > As an aside, I wonder if just these 3 priority classes will be sufficiently > granular to make good scheduling choices. > > Here for instance after the transition we could just be firing off all the tasks > with ExecutionMode::PARALLEL and priority=USER_VISIBLE. > > That is fine, but in theory we have more information available. Some of the > tasks (adapter probes) are less important than others and could be > de-prioritized. But not so unimportant as to become BACKGROUND. > > This particular case isn't all that interesting (wasn't really optimized > before), but still curious if there are plans to have more priorities or other > ways to describe precedence of tasks than those 3. Interesting, filed http://crbug.com/623683 to consider this.
Regarding other pools - it seems both Eric and I are uncertain what will happen w/r/t the WebCrypto thread, which similar may choke on mutexes held elsewhere, but... sure, let's see what happens. The SSLPlatformKey 'thread' is one which we know interacts with others in strange ways (including WebCrypto & UI) On 2016/06/27 21:29:32, gab wrote: > Got it, today there is PlatformThread::CreateNonJoinable(). Would a > base::Thread:Options that allows it to be non-joinable work for you? Seems to me > like it would achieve the same goal. We should follow-up in a separate CL, but that was one of the paths explored, and no, it did not work and revealed more complexity than desired, which is how we ended up at the present solution. > I'm surprised by the lack of comments on ssl_platform_key_task_runner.(cc|h) > explaining what seems like a very intricate design decision. Documenting all of the "How we got here" decisions doesn't usually increase readability, but hinders it. > I'm happy to put a CL together and work with you on this. Like I said, > SequencedWorkerPool will go away and its only use case can't remain something > which is inherently single-threaded. Let's follow-up separately then.
On Mon, Jun 27, 2016 at 2:58 PM, <rsleevi@chromium.org> wrote: > Regarding other pools - it seems both Eric and I are uncertain what will > happen > w/r/t the WebCrypto thread, which similar may choke on mutexes held > elsewhere, > but... sure, let's see what happens. The SSLPlatformKey 'thread' is one > which we > know interacts with others in strange ways (including WebCrypto & UI) > The WebCrypto case *should* be OK. That said, I don't think this particular Task Scheduler experiment is even going to be running for the Web Crypto code initially, since it doesn't sound like they are targeting the renderer process yet. > I spoke with David about the thread safety guarantees in BoringSSL around EVP_PKEY and its concurrent use across threads for varying operations -- it should be safe, and is explicitly in scope (provided you don't mutate the EVP_PKEYs). So if the thread affinity were to change between tasks (but still sequenced) or even if we went full-blown PARALLEL (crbug.com/623700) it should be fine. If not there is a bug to be fixed. Any internal locking in EVP_PKEY should be short lived. Also dropped some comment changes in https://codereview.chromium.org/2088323002/. I am fine with running this experiment. > > On 2016/06/27 21:29:32, gab wrote: > > Got it, today there is PlatformThread::CreateNonJoinable(). Would a > > base::Thread:Options that allows it to be non-joinable work for you? > Seems to > me > > like it would achieve the same goal. > > We should follow-up in a separate CL, but that was one of the paths > explored, > and no, it did not work and revealed more complexity than desired, which > is how > we ended up at the present solution. > > > I'm surprised by the lack of comments on > ssl_platform_key_task_runner.(cc|h) > > explaining what seems like a very intricate design decision. > > Documenting all of the "How we got here" decisions doesn't usually increase > readability, but hinders it. > > > I'm happy to put a CL together and work with you on this. Like I said, > > SequencedWorkerPool will go away and its only use case can't remain > something > > which is inherently single-threaded. > > Let's follow-up separately then. > > https://codereview.chromium.org/2077413009/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
base/trace_event LGTM
SimpleCache lgtm
base/threading/ LGTM https://codereview.chromium.org/2077413009/diff/60001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2077413009/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:1267: inner_(new Inner(this, is this git cl formatted?
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): nduca@chromium.org: - base/trace_event/memory_dump_manager_unittest.cc gene@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc gavinp@chromium.org: - net/disk_cache/simple/simple_backend_impl.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc Main API change, danakj@chromium.org: - base/threading/sequenced_worker_pool.cc - base/threading/sequenced_worker_pool.h BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): gene@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc BUG=553459, 622400 ==========
gab@chromium.org changed reviewers: + primiano@chromium.org - nduca@chromium.org
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): gene@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): gene@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc (usage to be removed in precursor CL) BUG=553459, 622400 ==========
Thanks, ping gene@ and jam@ (see reply above) https://codereview.chromium.org/2077413009/diff/60001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2077413009/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:1267: inner_(new Inner(this, On 2016/06/28 23:30:59, danakj wrote: > is this git cl formatted? Yes.
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): gene@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc (usage to be removed in precursor CL) BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc (usage to be removed in precursor CL) BUG=553459, 622400 ==========
gab@chromium.org changed reviewers: + scottbyer@chromium.org - gene@chromium.org
On 2016/06/29 18:25:04, gab wrote: > Thanks, > > ping gene@ and jam@ (see reply above) s/gene/scottbyer for chrome/service/service_process.cc (gene appears to have been inactive for a while? should he be removed from OWNERS?) > > https://codereview.chromium.org/2077413009/diff/60001/base/threading/sequence... > File base/threading/sequenced_worker_pool.cc (right): > > https://codereview.chromium.org/2077413009/diff/60001/base/threading/sequence... > base/threading/sequenced_worker_pool.cc:1267: inner_(new Inner(this, > On 2016/06/28 23:30:59, danakj wrote: > > is this git cl formatted? > > Yes.
On 2016/06/30 18:02:33, gab wrote: > On 2016/06/29 18:25:04, gab wrote: > > Thanks, > > > > ping gene@ and jam@ (see reply above) > > s/gene/scottbyer for chrome/service/service_process.cc (gene appears to have > been inactive for a while? should he be removed from OWNERS?) Also @rsleevi: added dependency on https://codereview.chromium.org/2103883004/ and thus this CL no longer touches ssl_platform_key_task_runner.cc, can you please undo your "not l.g.t.m."? > > > > > > https://codereview.chromium.org/2077413009/diff/60001/base/threading/sequence... > > File base/threading/sequenced_worker_pool.cc (right): > > > > > https://codereview.chromium.org/2077413009/diff/60001/base/threading/sequence... > > base/threading/sequenced_worker_pool.cc:1267: inner_(new Inner(this, > > On 2016/06/28 23:30:59, danakj wrote: > > > is this git cl formatted? > > > > Yes.
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc (usage to be removed in precursor CL) BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc BUG=553459, 622400 ==========
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc (moved to https://codereview.chromium.org/2103883004/ please remove not lgtm here) BUG=553459, 622400 ==========
lgtm
https://codereview.chromium.org/2077413009/diff/1/content/browser/browser_thr... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2077413009/diff/1/content/browser/browser_thr... content/browser/browser_thread_impl.cc:106: base::TaskPriority::BACKGROUND)) { On 2016/06/27 21:29:32, gab wrote: > On 2016/06/22 21:38:44, jam wrote: > > On 2016/06/22 21:08:56, gab (OOO back Monday) wrote: > > > On 2016/06/22 20:17:37, jam wrote: > > > > the blocking pool is for anything that needs to write to disk. some of > those > > > > tasks may be background, but not all. > > > > > > > > how did you arrive at this priority? > > > > > > IMO anything that goes in the BlockingPool in the browser shouldn't expect > any > > > kind of throughput. The critical disk access is on the CachePool, the > > > BlockingPool should only be used for background work. > > > > > > Or are you aware of different types of work going into the BlockingPool? > > > > In general I agree. However it's not clear that this is shared by all > consumers. > > Have you audited the 355 callers of this method? the documentation states: > > // Returns the thread pool used for blocking file I/O. Use this object to > > // perform random blocking operations such as file writes or querying the > > // Windows registry. > > > > It doesn't say that this is for things that "shouldn't expect any kind of > > throughput". If you'd like to change the behavior, that's fine, but the onus > > should be to first check existing callers. > > I don't think it's reasonable for any caller today to use the BlockingPool for > tasks that require responsiveness specifically because it documents itself as > being used to "perform random blocking operations" and as such even non-blocking > operations can be blocked behind other ones. The documentation for it in content/public/browser is that it should be used for blocking tasks such as file i/o or registry. That is in contrast to base::TaskPriority::BACKGROUND which is "user won't notice if this task takes an arbitrarily long time to complete." These two descriptions are not the same. There are 355 callers on https://cs.chromium.org/chromium/src/content/public/browser/browser_thread.h?.... I skipped the first two (ash) and the third(blimp). The first chrome one is drive_service_bridge.cc -this appears to affect the UI The second chrome one is in_memory_url_index_factory.cc -this clearly affects UI I stopped looking after. > > As such I don't think an audit is required as that would be a major undertaking > for an API which I honestly don't think should be used for the surveyed use > case.
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc rsleevi@chromium.org: - net/ssl/ssl_platform_key_task_runner.cc (moved to https://codereview.chromium.org/2103883004/ please remove not lgtm here) BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc BUG=553459, 622400 ==========
Patchset #8 (id:140001) has been deleted
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc Will find owner when bots happy: ios/* tools/gn/* BUG=553459, 622400 ==========
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc Will find owner when bots happy: ios/* tools/gn/* BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc brettw@chromium.org: - tools/gn/* sdefresne@chromium.org: - ios/* BUG=553459, 622400 ==========
gab@chromium.org changed reviewers: + brettw@chromium.org, sdefresne@chromium.org
@jam: you're right, done. Reviewers left: @scottbyer@chromium.org: - chrome/service/service_process.cc @jam@chromium.org: - content/browser/browser_thread_impl.cc (comment addressed) - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc New reviewers for latest changes: @brettw@chromium.org: - tools/gn/* @sdefresne@chromium.org: - ios/ Thanks, Gab https://codereview.chromium.org/2077413009/diff/1/content/browser/browser_thr... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/2077413009/diff/1/content/browser/browser_thr... content/browser/browser_thread_impl.cc:106: base::TaskPriority::BACKGROUND)) { On 2016/06/30 22:45:53, jam wrote: > On 2016/06/27 21:29:32, gab wrote: > > On 2016/06/22 21:38:44, jam wrote: > > > On 2016/06/22 21:08:56, gab (OOO back Monday) wrote: > > > > On 2016/06/22 20:17:37, jam wrote: > > > > > the blocking pool is for anything that needs to write to disk. some of > > those > > > > > tasks may be background, but not all. > > > > > > > > > > how did you arrive at this priority? > > > > > > > > IMO anything that goes in the BlockingPool in the browser shouldn't expect > > any > > > > kind of throughput. The critical disk access is on the CachePool, the > > > > BlockingPool should only be used for background work. > > > > > > > > Or are you aware of different types of work going into the BlockingPool? > > > > > > In general I agree. However it's not clear that this is shared by all > > consumers. > > > Have you audited the 355 callers of this method? the documentation states: > > > // Returns the thread pool used for blocking file I/O. Use this object to > > > // perform random blocking operations such as file writes or querying the > > > // Windows registry. > > > > > > It doesn't say that this is for things that "shouldn't expect any kind of > > > throughput". If you'd like to change the behavior, that's fine, but the onus > > > should be to first check existing callers. > > > > I don't think it's reasonable for any caller today to use the BlockingPool for > > tasks that require responsiveness specifically because it documents itself as > > being used to "perform random blocking operations" and as such even > non-blocking > > operations can be blocked behind other ones. > > The documentation for it in content/public/browser is that it should be used for > blocking tasks such as file i/o or registry. > > That is in contrast to base::TaskPriority::BACKGROUND which is "user won't > notice if this task takes an arbitrarily long time to complete." > > These two descriptions are not the same. > There are 355 callers on > https://cs.chromium.org/chromium/src/content/public/browser/browser_thread.h?.... > > I skipped the first two (ash) and the third(blimp). > The first chrome one is drive_service_bridge.cc > -this appears to affect the UI > The second chrome one is in_memory_url_index_factory.cc > -this clearly affects UI > > I stopped looking after. > > > > > As such I don't think an audit is required as that would be a major > undertaking > > for an API which I honestly don't think should be used for the surveyed use > > case. > Thanks for the examples, you're right. I guess that in the absence of a place for "foreground" file I/O (besides FILE_USER_BLOCKING thread in truly blocking scenarios), the BlockingPool has ended up mixing blocking background tasks with file tasks affecting UI elements. So for now USER_VISIBLE is required and this will be properly separated when deprecating the API in favor of explicit usage of TaskScheduler. Done.
ping: jam + scottbyer + brettw + sdefresne On 2016/07/05 13:53:44, gab wrote: > @jam: you're right, done. > > > Reviewers left: > > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=@scottbyer@chromium.org: > - chrome/service/service_process.cc > > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=@jam@chromium.org: > - content/browser/browser_thread_impl.cc (comment addressed) > - services/shell/standalone/context.cc > - services/shell/runner/host/child_process_host_unittest.cc > > > New reviewers for latest changes: > > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=@brettw@chromium.org: > - tools/gn/* > > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=@sdefresne@chromium.org: > - ios/ > > > Thanks, > Gab
Patchset #11 (id:220001) has been deleted
On 2016/07/07 14:49:58, gab wrote: > ping: jam + scottbyer + brettw + sdefresne lgtm sorry for the delay, just noticed this after vacation. in the future if i don't respond in a few hours feel free to IM me.
tools/gn lgtm
ios/ lgtm Sorry, completely missed the initial review request and the ping. Feel free to im/email if I'm unresponsive.
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc jam@chromium.org: - content/browser/browser_thread_impl.cc - services/shell/standalone/context.cc - services/shell/runner/host/child_process_host_unittest.cc brettw@chromium.org: - tools/gn/* sdefresne@chromium.org: - ios/* BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc BUG=553459, 622400 ==========
lgtm
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. Missing owner to stamp each change based on the above (will remove as lgtm's come in): scottbyer@chromium.org: - chrome/service/service_process.cc BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. BUG=553459, 622400 ==========
Thanks all, re-added old constructor (as deprecated) for few remaining callers (SSLPlatformKeyTaskRunner and OOMKillsMonitor; which will need to be manually migrated) in order to be able to land this.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, scheib@chromium.org, eroman@chromium.org, phajdan.jr@chromium.org, primiano@chromium.org, gavinp@chromium.org, danakj@chromium.org, rsleevi@chromium.org, jam@chromium.org, scottbyer@chromium.org, sdefresne@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2077413009/#ps260001 (title: "tweak comment")
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...
Message was sent while issue was closed.
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. BUG=553459, 622400 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. BUG=553459, 622400 ========== to ========== Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment. (ref. http://crbug.com/622400 for experiment details) The mapping should be as follows: - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to a user action. - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes) but can be re-ordered behind USER_BLOCKING work. - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere with foreground work. Since each pool can only be assigned a single priority (until we do the full migration and each TaskRunner extracted from it can have its own priority), the highest priority among all tasks running in that pool should be picked if the pool runs multiple types of tasks. Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler will likely soon replace those use cases. BUG=553459, 622400 Committed: https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56 Cr-Commit-Position: refs/heads/master@{#406481} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56 Cr-Commit-Position: refs/heads/master@{#406481} |
