|
|
DescriptionUse TaskScheduler instead of blocking pool in enumerate_modules_model.cc.
The blocking pool is being deprecated in favor of TaskScheduler.
BUG=667892
R=finnur@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Review-Url: https://codereview.chromium.org/2670763007
Cr-Commit-Position: refs/heads/master@{#452821}
Committed: https://chromium.googlesource.com/chromium/src/+/d4716407f7bc5093e820275b36d324aed39f8496
Patch Set 1 #Patch Set 2 : upload #
Total comments: 2
Patch Set 3 : headers order #Patch Set 4 : manual refactoring #
Total comments: 4
Patch Set 5 : CR and fix build error" #
Messages
Total messages: 33 (22 generated)
Description was changed from ========== Use TaskScheduler instead of blocking pool in enumerate_modules_model.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 enumerate_modules_model.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=finnur@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
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 :) .WithShutdownBehavior(base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). .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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
This code I'm familiar with and the change seems ok. But the errors on the Win bots are concerning, since this code is windows-only (and the logs are gone). Can you re-run the tests? https://codereview.chromium.org/2670763007/diff/20001/chrome/browser/win/enum... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2670763007/diff/20001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.cc:7: #include <mscat.h> // NOLINT: This must be after wincrypt and wintrust. Umm... this looks wrong...
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 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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
PTAnL I took the liberty of addressing the TODO on ScanImplDelay. https://codereview.chromium.org/2670763007/diff/20001/chrome/browser/win/enum... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2670763007/diff/20001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.cc:7: #include <mscat.h> // NOLINT: This must be after wincrypt and wintrust. On 2017/02/14 15:32:20, Finnur wrote: > Umm... this looks wrong... Sorry. sort-headers.py did a bad job here.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
> I took the liberty of addressing the TODO on ScanImplDelay. Thank you for that. I'm really close to giving this a green light. I don't see anything obviously wrong with this, except that the test EnumerateModulesTest.CollapsePath is failing. I do wonder, however, if we can verify somehow that the shutdown hang will not re-appear? Is there a pre-existing test to verify this or if not, can it be done by hand? Or is the strategy to wait and see how this does in the canary? https://codereview.chromium.org/2670763007/diff/60001/chrome/browser/win/enum... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2670763007/diff/60001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.cc:171: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN))), Speaking out loud here -- I really don't like the ambiguous name of this enum value. It could be referring to what Chrome does on shutdown (continue and ignore the task) or it could be referring to how the task behaves on shutdown (continues running). It becomes a little clearer when you look at the comments for this enum and how the rest of the enums are named, but we could skip all that reading and make it less ambigious if we just called it DONT_BLOCK_SHUTDOWN. That doesn't block* this CL, though. * no pun intended. https://codereview.chromium.org/2670763007/diff/60001/chrome/browser/win/enum... File chrome/browser/win/enumerate_modules_model.h (right): https://codereview.chromium.org/2670763007/diff/60001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.h:342: // the |module_enumerator_|, read from on the UI thread by this class. nit: These two lines can be consolidated.
Also, apologies for the way. This arrived after dinner on Friday and I took a vacation day yesterday (Monday).
On 2017/02/21 10:47:12, Finnur wrote: > Also, apologies for the way. This arrived after dinner on Friday and I took a > vacation day yesterday (Monday). ... for the wait* (not way)
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...
> I do wonder, however, if we can verify somehow that the shutdown hang will not > re-appear? Is there a pre-existing test to verify this or if not, can it be done > by hand? Or is the strategy to wait and see how this does in the canary? TaskScheduler does not wait for CONTINUE_ON_SHUTDOWN tasks on shutdown. If you really want to, we could add ModuleEnumerator::SetScanImplModuleEventForTesting(WaitableEvent* event) that would cause ScanImplModule() to wait for an event. We could then verify that TaskScheduler::GetInstance()->Shutdown() returns if the event is not signaled. But I'm not convinced that it is worth it to verify that TaskScheduler handles CONTINUE_ON_SHUTDOWN correctly. https://codereview.chromium.org/2670763007/diff/60001/chrome/browser/win/enum... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2670763007/diff/60001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.cc:171: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN))), On 2017/02/21 10:45:55, Finnur wrote: > Speaking out loud here -- I really don't like the ambiguous name of this enum > value. It could be referring to what Chrome does on shutdown (continue and > ignore the task) or it could be referring to how the task behaves on shutdown > (continues running). It becomes a little clearer when you look at the comments > for this enum and how the rest of the enums are named, but we could skip all > that reading and make it less ambigious if we just called it > DONT_BLOCK_SHUTDOWN. That doesn't block* this CL, though. > > * no pun intended. I agree with you. I'll show your comment to my team :) https://codereview.chromium.org/2670763007/diff/60001/chrome/browser/win/enum... File chrome/browser/win/enumerate_modules_model.h (right): https://codereview.chromium.org/2670763007/diff/60001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.h:342: // the |module_enumerator_|, read from on the UI thread by this class. On 2017/02/21 10:45:55, Finnur wrote: > nit: These two lines can be consolidated. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. > If you really want to, we could add ModuleEnumerator::SetScanImplModuleEventForTesting No, that's fine. I wasn't arguing for adding more code. I was just curious if there's a way to test this scenario manually and if not, are there obvious signs you can monitor for on the canary to make sure it doesn't regress?
On 2017/02/24 13:25:03, Finnur wrote: > LGTM. > > > If you really want to, we could add > ModuleEnumerator::SetScanImplModuleEventForTesting > > No, that's fine. I wasn't arguing for adding more code. I was just curious if > there's a way to test this scenario manually and if not, are there obvious signs > you can monitor for on the canary to make sure it doesn't regress? We can monitor shutdown hangs.
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": 80001, "attempt_start_ts": 1487943157121230, "parent_rev": "8167aa05471108079a7f8aefae5135b037c92f45", "commit_rev": "d4716407f7bc5093e820275b36d324aed39f8496"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of blocking pool in enumerate_modules_model.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=finnur@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Use TaskScheduler instead of blocking pool in enumerate_modules_model.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=finnur@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2670763007 Cr-Commit-Position: refs/heads/master@{#452821} Committed: https://chromium.googlesource.com/chromium/src/+/d4716407f7bc5093e820275b36d3... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/d4716407f7bc5093e820275b36d3... |