|
|
Created:
4 years, 3 months ago by gab Modified:
4 years, 3 months ago Reviewers:
Matt Giuca CC:
chromium-reviews, tfarina, Matt Giuca Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool multi-threaded.
BUG=646443
Committed: https://crrev.com/eb5fb080ec2c32864e7da2a2c1a3783120eafa1e
Cr-Commit-Position: refs/heads/master@{#419085}
Patch Set 1 #Patch Set 2 : +HistoryDataStoreTest #Patch Set 3 : #
Total comments: 4
Messages
Total messages: 21 (13 generated)
Description was changed from ========== Make SearchHistoryTest's SequencedWorkerPool multi-threaded. BUG=646443 ========== to ========== Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool multi-threaded. BUG=646443 ==========
gab@chromium.org changed reviewers: + mgiuca@chromium.org
Matt PTaL, thanks!
The CQ bit was checked by gab@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gab@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 2016/09/14 19:57:08, gab wrote: > Matt PTaL, thanks! ping mgiuca@, thanks!
https://codereview.chromium.org/2345583002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/history_unittest.cc (right): https://codereview.chromium.org/2345583002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/history_unittest.cc:99: new base::SequencedWorkerPoolOwner(2, "AppLauncherTest")); I'm not really familiar with this code. Is there a good reason it has to be multi-threaded in a unit test? It is probably not going to change anything, but it means there's a new possibility of flakiness (things happening out of order) that weren't flaky before. https://codereview.chromium.org/2345583002/diff/40001/ui/app_list/search/hist... File ui/app_list/search/history_data_store_unittest.cc (right): https://codereview.chromium.org/2345583002/diff/40001/ui/app_list/search/hist... ui/app_list/search/history_data_store_unittest.cc:44: HistoryDataStoreTest() : worker_pool_owner_(2, "AppLanucherTest") {} Same.
https://codereview.chromium.org/2345583002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/history_unittest.cc (right): https://codereview.chromium.org/2345583002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/history_unittest.cc:99: new base::SequencedWorkerPoolOwner(2, "AppLauncherTest")); On 2016/09/15 23:54:20, Matt Giuca wrote: > I'm not really familiar with this code. Is there a good reason it has to be > multi-threaded in a unit test? > > It is probably not going to change anything, but it means there's a new > possibility of flakiness (things happening out of order) that weren't flaky > before. In this test it's used to pass to a DictionaryDataStore (https://cs.chromium.org/chromium/src/ui/app_list/search/dictionary_data_store...) which in production receives a real (multi-threaded) SequencedWorkerPool. So if this results in flakes it would mean DictionaryDataStore doesn't probably use a SequencedWorkerPool. i.e. if anything this brings the test closer to what runs in production. Making the SWP multi-threaded (i.e. max_threads > 1) just means it's allowed to have multiple threads, but if the caller only uses one sequence things are still going to be sequenced on a single thread -- if that's the case this change is a no-op in practice (but we want to do it regardless as we're looking to disallow single-threaded SequencedWorkerPools so that assuming thread-affinity from a SWP is never okay, users that want that should use base::Thread). https://codereview.chromium.org/2345583002/diff/40001/ui/app_list/search/hist... File ui/app_list/search/history_data_store_unittest.cc (right): https://codereview.chromium.org/2345583002/diff/40001/ui/app_list/search/hist... ui/app_list/search/history_data_store_unittest.cc:44: HistoryDataStoreTest() : worker_pool_owner_(2, "AppLanucherTest") {} On 2016/09/15 23:54:20, Matt Giuca wrote: > Same. Same.
OK thanks for the explanation, lgtm.
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 ========== Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool multi-threaded. BUG=646443 ========== to ========== Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool multi-threaded. BUG=646443 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool multi-threaded. BUG=646443 ========== to ========== Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool multi-threaded. BUG=646443 Committed: https://crrev.com/eb5fb080ec2c32864e7da2a2c1a3783120eafa1e Cr-Commit-Position: refs/heads/master@{#419085} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/eb5fb080ec2c32864e7da2a2c1a3783120eafa1e Cr-Commit-Position: refs/heads/master@{#419085} |