Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2672623006:
Use TaskScheduler instead of blocking pool in content_hash_fetcher.cc. (Closed)
DescriptionUse TaskScheduler instead of blocking pool in content_hash_fetcher.cc.
The blocking pool is being deprecated in favor of TaskScheduler.
BUG=667892
R=finnur@chromium.org
Review-Url: https://codereview.chromium.org/2672623006
Cr-Original-Commit-Position: refs/heads/master@{#451473}
Committed: https://chromium.googlesource.com/chromium/src/+/8a07a9568d954e17452e678852692838a613f3c1
Review-Url: https://codereview.chromium.org/2672623006
Cr-Commit-Position: refs/heads/master@{#463012}
Committed: https://chromium.googlesource.com/chromium/src/+/92f436707228df9a29f1e9bd9c9dddb1412cf4be
Patch Set 1 #Patch Set 2 : upload #
Total comments: 1
Patch Set 3 : USER_VISIBLE #
Total comments: 1
Patch Set 4 : USER_VISIBLE #Messages
Total messages: 29 (15 generated)
Hi! I'm a Python script responsible for migrating tasks from the blocking pool to TaskScheduler. In this CL, I used these traits to post to TaskScheduler a task that was previously posted to the blocking pool: .WithPriority(base::TaskPriority::BACKGROUND) User won't notice if the task takes an arbitrarily long time to complete. Making your task BACKGROUND allows non-BACKGROUND tasks to run faster :) *No* .WithShutdownBehavior() .MayBlock() The task may block. This includes but is not limited to tasks that wait on synchronous file I/O operations: read or write a file from disk, interact with a pipe or a socket, rename or delete a file, enumerate files in a directory, etc. This trait isn't required for the mere use of locks. *No* .WithBaseSyncPrimitives() The task is not allowed to use these functions: - base::WaitableEvent::Wait - base::ConditionVariable::Wait - base::PlatformThread::Join - base::PlatformThread::Sleep - base::Process::WaitForExit - base::Process::WaitForExitWithTimeout If you think this is correct, please LGTM and CQ this CL. Otherwise, tell me which traits I should have used and I'll automatically upload a new patch. You can find documentation at https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h WithPriority() [default/BACKGROUND/USER_VISIBLE/USER_BLOCKING]: WithShutdownBehavior() [default/CONTINUE_ON_SHUTDOWN/SKIP_ON_SHUTDOWN/BLOCK_SHUTDOWN]: MayBlock() [yes/no]: WithBaseSyncPrimitives() [yes/no]: Comments for a human: [A human will read this] FAQ: What should I do if the CQ dry run didn't pass? You can ignore this CL. A human will take a look and get back to you. Why are you doing this? Browser threads, the blocking pool and base::WorkerPool are being deprecated in favor of TaskScheduler. Design doc: https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... What is the default priority? A task posted to TaskScheduler without an explicit priority inherits its priority from the calling context (e.g. a task posted to TaskScheduler from a BACKGROUND task without an explicit .WithPriority() will have a BACKGROUND priority). What is the default shutdown behavior? It is not documented on purpose. If your task shouldn't be skipped on shutdown (e.g. a task that persists user data on disk), it should have an explicit BLOCK_SHUTDOWN shutdown behavior. If it's important not to wait on your task on shutdown (e.g. it takes a long time to run and could cause a shutdown hang), it should have an explicit CONTINUE_ON_SHUTDOWN shutdown behavior.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/03 18:06:12, fdoray wrote: > Hi! I'm a Python script responsible for migrating tasks from the > blocking pool to TaskScheduler. In this CL, I used these traits to post to > TaskScheduler a task that was previously posted to the blocking pool: > > .WithPriority(base::TaskPriority::BACKGROUND) > User won't notice if the task takes an arbitrarily long time to complete. > Making your task BACKGROUND allows non-BACKGROUND tasks to run faster :) > > *No* .WithShutdownBehavior() > > .MayBlock() > The task may block. This includes but is not limited to tasks that wait on > synchronous file I/O operations: read or write a file from disk, interact > with a pipe or a socket, rename or delete a file, enumerate files in a > directory, etc. This trait isn't required for the mere use of locks. > > *No* .WithBaseSyncPrimitives() > The task is not allowed to use these functions: > - base::WaitableEvent::Wait > - base::ConditionVariable::Wait > - base::PlatformThread::Join > - base::PlatformThread::Sleep > - base::Process::WaitForExit > - base::Process::WaitForExitWithTimeout > > If you think this is correct, please LGTM and CQ this CL. > > Otherwise, tell me which traits I should have used and I'll automatically > upload a new patch. You can find documentation at > https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h > > WithPriority() > [default/BACKGROUND/USER_VISIBLE/USER_BLOCKING]: > > WithShutdownBehavior() > [default/CONTINUE_ON_SHUTDOWN/SKIP_ON_SHUTDOWN/BLOCK_SHUTDOWN]: > > MayBlock() > [yes/no]: > > WithBaseSyncPrimitives() > [yes/no]: > > Comments for a human: > [A human will read this] > > FAQ: > > What should I do if the CQ dry run didn't pass? > You can ignore this CL. A human will take a look and get back to you. > > Why are you doing this? > Browser threads, the blocking pool and base::WorkerPool are being deprecated > in favor of TaskScheduler. Design doc: > https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... > > What is the default priority? > A task posted to TaskScheduler without an explicit priority inherits its > priority from the calling context (e.g. a task posted to TaskScheduler > from a BACKGROUND task without an explicit .WithPriority() will have a > BACKGROUND priority). > > What is the default shutdown behavior? > It is not documented on purpose. If your task shouldn't be skipped on shutdown > (e.g. a task that persists user data on disk), it should have an explicit > BLOCK_SHUTDOWN shutdown behavior. If it's important not to wait on your task > on shutdown (e.g. it takes a long time to run and could cause a shutdown > hang), it should have an explicit CONTINUE_ON_SHUTDOWN shutdown behavior. ping
Description was changed from ========== Use TaskScheduler instead of blocking pool in content_hash_fetcher.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=finnur@chromium.org ========== to ========== Use TaskScheduler instead of blocking pool in content_hash_fetcher.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=finnur@chromium.org ==========
finnur@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - finnur@chromium.org
Devlin is a better candidate for these reviews.
https://codereview.chromium.org/2672623006/diff/20001/extensions/browser/cont... File extensions/browser/content_hash_fetcher.cc (right): https://codereview.chromium.org/2672623006/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:200: base::TaskPriority::BACKGROUND), WithPriority() USER_VISIBLE This is in response to loading an extension, and may result in the extension becoming disabled or notifying the user of corruption.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAnL
https://codereview.chromium.org/2672623006/diff/40001/extensions/browser/cont... File extensions/browser/content_hash_fetcher.cc (right): https://codereview.chromium.org/2672623006/diff/40001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:292: base::TaskPriority::BACKGROUND), ditto: USER_VISIBLE This is in response to loading an extension, and may result in the extension becoming disabled or notifying the user of corruption. (Would have mentioned this in the previous patch set, but this wasn't changed in PS2)
PTAnL
lgtm
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487434706415540,
"parent_rev": "88bda6e478572197c7c1dcf6bc6fb0b7b3f1e586", "commit_rev":
"8a07a9568d954e17452e678852692838a613f3c1"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of blocking pool in content_hash_fetcher.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=finnur@chromium.org ========== to ========== Use TaskScheduler instead of blocking pool in content_hash_fetcher.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=finnur@chromium.org Review-Url: https://codereview.chromium.org/2672623006 Cr-Commit-Position: refs/heads/master@{#451473} Committed: https://chromium.googlesource.com/chromium/src/+/8a07a9568d954e17452e67885269... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/8a07a9568d954e17452e67885269...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2787453003/ by fdoray@chromium.org. The reason for reverting is: Causing flakyness https://crbug.com/702300.
Message was sent while issue was closed.
On 2017/03/29 13:46:30, fdoray wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/2787453003/ by mailto:fdoray@chromium.org. > > The reason for reverting is: Causing flakyness https://crbug.com/702300. Relanding since reverting didn't fix the issue described in https://crbug.com/702300
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of blocking pool in content_hash_fetcher.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=finnur@chromium.org Review-Url: https://codereview.chromium.org/2672623006 Cr-Commit-Position: refs/heads/master@{#451473} Committed: https://chromium.googlesource.com/chromium/src/+/8a07a9568d954e17452e67885269... ========== to ========== Use TaskScheduler instead of blocking pool in content_hash_fetcher.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=finnur@chromium.org Review-Url: https://codereview.chromium.org/2672623006 Cr-Commit-Position: refs/heads/master@{#451473} Committed: https://chromium.googlesource.com/chromium/src/+/8a07a9568d954e17452e67885269... ==========
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491597182671630,
"parent_rev": "b3e132a8f0a9aa4fe17aaa90246d09efd05cfcc3", "commit_rev":
"92f436707228df9a29f1e9bd9c9dddb1412cf4be"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of blocking pool in content_hash_fetcher.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=finnur@chromium.org Review-Url: https://codereview.chromium.org/2672623006 Cr-Commit-Position: refs/heads/master@{#451473} Committed: https://chromium.googlesource.com/chromium/src/+/8a07a9568d954e17452e67885269... ========== to ========== Use TaskScheduler instead of blocking pool in content_hash_fetcher.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=finnur@chromium.org Review-Url: https://codereview.chromium.org/2672623006 Cr-Original-Commit-Position: refs/heads/master@{#451473} Committed: https://chromium.googlesource.com/chromium/src/+/8a07a9568d954e17452e67885269... Review-Url: https://codereview.chromium.org/2672623006 Cr-Commit-Position: refs/heads/master@{#463012} Committed: https://chromium.googlesource.com/chromium/src/+/92f436707228df9a29f1e9bd9c9d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/92f436707228df9a29f1e9bd9c9d... |
