Disallow single-threaded SequencedWorkerPools |
|||
Issue descriptionr409668 and r411899 were already landed in that direction as a requirement for issue 622400 (base::Thread and base::SimpleThread now have a non-joinable option which fulfills the last reason to use a single-threaded SWP). Having no single-threaded SWP will make the SWP redirection to TaskScheduler simpler. There are only two use cases left: 1) image_fetcher_unittest.mm which could use a base::Thread and obtain the same result. 2) WebCrypto which can use a base::Thread for now (and can later move to TaskScheduler on its own schedule as tracked by issue 623700).
,
Sep 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fe6ad6084dd2680ce5c5247b627bdd9dbe7c27ff commit fe6ad6084dd2680ce5c5247b627bdd9dbe7c27ff Author: gab <gab@chromium.org> Date: Wed Sep 14 21:10:44 2016 Make TestRLZTrackerDelegate's SequencedWorkerPool multi-threaded. BUG= 646443 Review-Url: https://codereview.chromium.org/2340063003 Cr-Commit-Position: refs/heads/master@{#418674} [modify] https://crrev.com/fe6ad6084dd2680ce5c5247b627bdd9dbe7c27ff/components/rlz/rlz_tracker_unittest.cc
,
Sep 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8106bcd1f350281fc46095441c1e994b003901e7 commit 8106bcd1f350281fc46095441c1e994b003901e7 Author: gab <gab@chromium.org> Date: Thu Sep 15 13:34:53 2016 Make SQLitePersistentCookieStorePerfTest's SequencedWorkerPool multi-threaded. BUG= 646443 Review-Url: https://codereview.chromium.org/2344593003 Cr-Commit-Position: refs/heads/master@{#418854} [modify] https://crrev.com/8106bcd1f350281fc46095441c1e994b003901e7/net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc
,
Sep 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2525299ceb48722a1271c8c7e8ae1bbd8aa724e3 commit 2525299ceb48722a1271c8c7e8ae1bbd8aa724e3 Author: gab <gab@chromium.org> Date: Thu Sep 15 13:35:42 2016 Replace single-threaded SequencedWorkerPool with base::Thread in WallpaperResizerTest. Also while here: - const scoped_refptr<>& => scoped_refptr<> + std::move (helps avoiding unnecessary refcount bumps: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/TlL1D-Djta0/discussion) BUG= 646443 Review-Url: https://codereview.chromium.org/2338363002 Cr-Commit-Position: refs/heads/master@{#418855} [modify] https://crrev.com/2525299ceb48722a1271c8c7e8ae1bbd8aa724e3/components/wallpaper/wallpaper_resizer.cc [modify] https://crrev.com/2525299ceb48722a1271c8c7e8ae1bbd8aa724e3/components/wallpaper/wallpaper_resizer.h [modify] https://crrev.com/2525299ceb48722a1271c8c7e8ae1bbd8aa724e3/components/wallpaper/wallpaper_resizer_unittest.cc
,
Sep 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/22327e7a3945aec88c8a1669b2da9aabefe13d88 commit 22327e7a3945aec88c8a1669b2da9aabefe13d88 Author: gab <gab@chromium.org> Date: Thu Sep 15 18:35:10 2016 Replace single-threaded SequencedWorkerPool with base::Thread in net_unittests Only logic change is that base::Thread will run all tasks before being destroyed (as opposed to previous SKIP_ON_SHUTDOWN semantics), but no tests called Shutdown() explicitly so I assume this wasn't an explicit choice and that those tests don't care? A few drive-by cleanups too. BUG= 646443 NO_DEPENDENCY_CHECKS=true Review-Url: https://codereview.chromium.org/2334163005 Cr-Commit-Position: refs/heads/master@{#418912} [modify] https://crrev.com/22327e7a3945aec88c8a1669b2da9aabefe13d88/net/base/file_stream_unittest.cc [modify] https://crrev.com/22327e7a3945aec88c8a1669b2da9aabefe13d88/net/url_request/url_request_simple_job_unittest.cc
,
Sep 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9954e8c35b3a0228c6bf10f712921514da491478 commit 9954e8c35b3a0228c6bf10f712921514da491478 Author: gab <gab@chromium.org> Date: Thu Sep 15 21:28:44 2016 Remove explicit usage of SequencedWorkerPool from UploadList. It merely needs a TaskRunner to PostTask() to. This allows the unittest to merely use a base::Thread and alleviates issue 646443 . Also did some modern style cleanup: - ThreadChecker => SequenceChecker (no state is thread-affine) - const scoped_refptr<>& => scoped_refptr<> + std::move (avoids a refcount bump when handing off ownership: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/TlL1D-Djta0/discussion) BUG= 646443 NO_DEPENDENCY_CHECKS=true Review-Url: https://codereview.chromium.org/2335193007 Cr-Commit-Position: refs/heads/master@{#418981} [modify] https://crrev.com/9954e8c35b3a0228c6bf10f712921514da491478/chrome/browser/crash_upload_list/crash_upload_list.cc [modify] https://crrev.com/9954e8c35b3a0228c6bf10f712921514da491478/chrome/browser/crash_upload_list/crash_upload_list_android.cc [modify] https://crrev.com/9954e8c35b3a0228c6bf10f712921514da491478/chrome/browser/crash_upload_list/crash_upload_list_android.h [modify] https://crrev.com/9954e8c35b3a0228c6bf10f712921514da491478/chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc [modify] https://crrev.com/9954e8c35b3a0228c6bf10f712921514da491478/chrome/browser/crash_upload_list/crash_upload_list_crashpad.h [modify] https://crrev.com/9954e8c35b3a0228c6bf10f712921514da491478/components/upload_list/crash_upload_list.cc [modify] https://crrev.com/9954e8c35b3a0228c6bf10f712921514da491478/components/upload_list/crash_upload_list.h [modify] https://crrev.com/9954e8c35b3a0228c6bf10f712921514da491478/components/upload_list/upload_list.cc [modify] https://crrev.com/9954e8c35b3a0228c6bf10f712921514da491478/components/upload_list/upload_list.h [modify] https://crrev.com/9954e8c35b3a0228c6bf10f712921514da491478/components/upload_list/upload_list_unittest.cc
,
Sep 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/54ef83562c4846bba5b4a7f923b15410eefd1a0f commit 54ef83562c4846bba5b4a7f923b15410eefd1a0f Author: gab <gab@chromium.org> Date: Thu Sep 15 23:04:08 2016 Add FlushForTesting to base::Thread. Grew tired of the post+wait pattern in many tests to flush a base::Thread. Was going to need it again in https://codereview.chromium.org/2333023003/ and decided to add an API for everyone's sake instead :-). Off the top of my head, will also be useful in https://codereview.chromium.org/2299523003/ BUG= 646443 Review-Url: https://codereview.chromium.org/2337253003 Cr-Commit-Position: refs/heads/master@{#419026} [modify] https://crrev.com/54ef83562c4846bba5b4a7f923b15410eefd1a0f/base/threading/thread.cc [modify] https://crrev.com/54ef83562c4846bba5b4a7f923b15410eefd1a0f/base/threading/thread.h [modify] https://crrev.com/54ef83562c4846bba5b4a7f923b15410eefd1a0f/base/threading/thread_unittest.cc
,
Sep 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cab88d00953b328b6ee4e4aa285a686da79cfb57 commit cab88d00953b328b6ee4e4aa285a686da79cfb57 Author: gab <gab@chromium.org> Date: Fri Sep 16 00:20:17 2016 Replace usage of single-threaded SequencedWorkerPool with a base::Thread in image_fetcher_unittest.mm Also cleaned up a TODO for 440857 on the way. BUG= 646443 , 440857 Review-Url: https://codereview.chromium.org/2333023003 Cr-Commit-Position: refs/heads/master@{#419049} [modify] https://crrev.com/cab88d00953b328b6ee4e4aa285a686da79cfb57/ios/chrome/browser/net/image_fetcher_unittest.mm
,
Sep 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ee46aaa20cac1fa414809d49c762bc284863020d commit ee46aaa20cac1fa414809d49c762bc284863020d Author: gab <gab@chromium.org> Date: Fri Sep 16 01:25:06 2016 Make DefaultComponentInstallerTest's SequencedWorkerPool multi-threaded. BUG= 646443 NO_DEPENDENCY_CHECKS=true Review-Url: https://codereview.chromium.org/2338333002 Cr-Commit-Position: refs/heads/master@{#419070} [modify] https://crrev.com/ee46aaa20cac1fa414809d49c762bc284863020d/components/component_updater/default_component_installer_unittest.cc
,
Sep 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb5fb080ec2c32864e7da2a2c1a3783120eafa1e commit eb5fb080ec2c32864e7da2a2c1a3783120eafa1e Author: gab <gab@chromium.org> Date: Fri Sep 16 03:02:25 2016 Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool multi-threaded. BUG= 646443 Review-Url: https://codereview.chromium.org/2345583002 Cr-Commit-Position: refs/heads/master@{#419085} [modify] https://crrev.com/eb5fb080ec2c32864e7da2a2c1a3783120eafa1e/chrome/browser/ui/app_list/search/history_unittest.cc [modify] https://crrev.com/eb5fb080ec2c32864e7da2a2c1a3783120eafa1e/ui/app_list/search/history_data_store_unittest.cc
,
Sep 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4a82ab69af2831779d3f5fc9a742cca75c415741 commit 4a82ab69af2831779d3f5fc9a742cca75c415741 Author: gab <gab@chromium.org> Date: Fri Sep 16 12:38:37 2016 Make SyncEngineTest's SequencedWorkerPool multi-threaded. BUG= 646443 Review-Url: https://codereview.chromium.org/2339253004 Cr-Commit-Position: refs/heads/master@{#419149} [modify] https://crrev.com/4a82ab69af2831779d3f5fc9a742cca75c415741/chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc
,
Sep 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/17d49a70d15f0084de6006afe9bc7356d0bdcdea commit 17d49a70d15f0084de6006afe9bc7356d0bdcdea Author: gab <gab@chromium.org> Date: Fri Sep 16 19:24:11 2016 Use base::Thread instead of single-threaded SequencedWorkerPool in TestLauncher. BUG= 646443 Review-Url: https://codereview.chromium.org/2342403002 Cr-Commit-Position: refs/heads/master@{#419247} [modify] https://crrev.com/17d49a70d15f0084de6006afe9bc7356d0bdcdea/base/test/launcher/test_launcher.cc [modify] https://crrev.com/17d49a70d15f0084de6006afe9bc7356d0bdcdea/base/test/launcher/test_launcher.h
,
Sep 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/37151e9412c1676e78063eed81568c62646724e9 commit 37151e9412c1676e78063eed81568c62646724e9 Author: gab <gab@chromium.org> Date: Mon Sep 19 17:47:47 2016 Disallow single-threaded SequencedWorkerPools. BUG= 646443 Review-Url: https://codereview.chromium.org/2330303003 Cr-Commit-Position: refs/heads/master@{#419496} [modify] https://crrev.com/37151e9412c1676e78063eed81568c62646724e9/base/threading/sequenced_worker_pool.cc [modify] https://crrev.com/37151e9412c1676e78063eed81568c62646724e9/base/threading/sequenced_worker_pool.h
,
Sep 19 2016
:-)
,
Nov 7 2016
,
Jan 6 2017
GN uses a single-threaded worker pool for debugging purposes. I noticed this when trying to run GN with --threads=1. Forcing one worker thread makes the GN loader and interpreter single threaded which is important for many common debugging tasks. It seems very reasonable that one would want a single-threaded worker pool for debugging purposes. Can you go into more detail about why this needs to be disallowed via an assertion? I would like to remove the assertion to support this use-case again.
,
Jan 9 2017
Ah interesting, we added this restriction because previously some components were using single-threaded SequencedWorkerPool in thread-affine scenarios. Those were updated to use a non-joinable base::Thread as it felt wrong to post thread-affine work to a SequencedWorkerPool (plus it was going to be a pain to support SINGLE_THREADED work from SequencedWorkerPools in the TaskScheduler redirection). For debugging purposes I can see how this makes sense though (and we do similarly have a single-threaded TaskScheduler for testing). I'm fine removing this restriction so long as the constructor's documentation is clear that |max_threads == 1| can be used for debugging purposes bug not to create a thread-affine SWP. |
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Sep 14 2016