|
|
Created:
3 years, 5 months ago by Ilya Sherman Modified:
3 years, 5 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Cleanup] Migrate the WatcherMetricsProvider to use the Task Scheduler.
BUG=667892
TEST=none
R=asvitkine@chromium.org
Review-Url: https://codereview.chromium.org/2963573004
Cr-Commit-Position: refs/heads/master@{#484860}
Committed: https://chromium.googlesource.com/chromium/src/+/1caa0802385fff5ac048cfe1423c19ab3a72acdf
Patch Set 1 #Patch Set 2 : Fix unittest #Patch Set 3 : Bottleneck through a single sequence for access to the registry key #
Total comments: 5
Patch Set 4 : add missing namespace #Patch Set 5 : initialize the task runner #Patch Set 6 : Fix threading in tests? #
Total comments: 1
Patch Set 7 : Document that registry interactions can block, and adjust the unittest not to be flaky #
Messages
Total messages: 50 (34 generated)
The CQ bit was checked by isherman@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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Will wait for green bots.
The CQ bit was checked by isherman@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 isherman@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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by isherman@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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by isherman@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 isherman@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.
isherman@chromium.org changed reviewers: + manzagop@chromium.org
https://codereview.chromium.org/2963573004/diff/40001/components/browser_watc... File components/browser_watcher/watcher_metrics_provider_win_unittest.cc (right): https://codereview.chromium.org/2963573004/diff/40001/components/browser_watc... components/browser_watcher/watcher_metrics_provider_win_unittest.cc:139: key.Open(HKEY_CURRENT_USER, kRegistryPath, KEY_READ)); It's not clear to me whether this is appropriate to test with the task scheduler, nor that this works correctly in the task scheduler world, as the code might run in parallel. I'm inclined to simply remove this check; but we could also instead substitute a test-specific task runner, and check this property using that task runner (similar to how this test used to work). WDYT?
lgtm https://codereview.chromium.org/2963573004/diff/40001/components/browser_watc... File components/browser_watcher/watcher_metrics_provider_win_unittest.cc (right): https://codereview.chromium.org/2963573004/diff/40001/components/browser_watc... components/browser_watcher/watcher_metrics_provider_win_unittest.cc:139: key.Open(HKEY_CURRENT_USER, kRegistryPath, KEY_READ)); On 2017/06/30 18:29:41, Ilya Sherman wrote: > It's not clear to me whether this is appropriate to test with the task > scheduler, nor that this works correctly in the task scheduler world, as the > code might run in parallel. I'm inclined to simply remove this check; but we > could also instead substitute a test-specific task runner, and check this > property using that task runner (similar to how this test used to work). WDYT? Could this simply go to l.127? https://codereview.chromium.org/2963573004/diff/100001/components/browser_wat... File components/browser_watcher/watcher_metrics_provider_win.cc (right): https://codereview.chromium.org/2963573004/diff/100001/components/browser_wat... components/browser_watcher/watcher_metrics_provider_win.cc:244: if (user_data_dir_.empty() || crash_dir_.empty()) { Sidenote, this reminds me I have a bug to make this a free function to avoid a UAF (https://crbug.com/711743).
https://codereview.chromium.org/2963573004/diff/40001/components/browser_watc... File components/browser_watcher/watcher_metrics_provider_win_unittest.cc (right): https://codereview.chromium.org/2963573004/diff/40001/components/browser_watc... components/browser_watcher/watcher_metrics_provider_win_unittest.cc:139: key.Open(HKEY_CURRENT_USER, kRegistryPath, KEY_READ)); On 2017/06/30 20:11:43, manzagop (departed) wrote: > On 2017/06/30 18:29:41, Ilya Sherman wrote: > > It's not clear to me whether this is appropriate to test with the task > > scheduler, nor that this works correctly in the task scheduler world, as the > > code might run in parallel. I'm inclined to simply remove this check; but we > > could also instead substitute a test-specific task runner, and check this > > property using that task runner (similar to how this test used to work). > WDYT? > > Could this simply go to l.127? It could, but it would be changing the meaning of the test. The comment indicates that this is currently testing that the provider schedules the work on a background thread, rather than performing it synchronously. Is this something that we still want to test? And if not, is there value in preserving this check to test something different -- e.g. that the provider does delete the key?
https://codereview.chromium.org/2963573004/diff/40001/components/browser_watc... File components/browser_watcher/watcher_metrics_provider_win_unittest.cc (right): https://codereview.chromium.org/2963573004/diff/40001/components/browser_watc... components/browser_watcher/watcher_metrics_provider_win_unittest.cc:139: key.Open(HKEY_CURRENT_USER, kRegistryPath, KEY_READ)); On 2017/07/03 02:35:07, Ilya Sherman wrote: > On 2017/06/30 20:11:43, manzagop (departed) wrote: > > On 2017/06/30 18:29:41, Ilya Sherman wrote: > > > It's not clear to me whether this is appropriate to test with the task > > > scheduler, nor that this works correctly in the task scheduler world, as the > > > code might run in parallel. I'm inclined to simply remove this check; but > we > > > could also instead substitute a test-specific task runner, and check this > > > property using that task runner (similar to how this test used to work). > > WDYT? > > > > Could this simply go to l.127? > > It could, but it would be changing the meaning of the test. The comment > indicates that this is currently testing that the provider schedules the work on > a background thread, rather than performing it synchronously. Is this something > that we still want to test? And if not, is there value in preserving this check > to test something different -- e.g. that the provider does delete the key? Ah, got it. So deletion is the intent and the posting on background is a proxy for not holding up the rest too much? I don't feel strongly about testing the later. Perhaps a simple comment at the posting site to mention the concern about the task being expensive?
Thanks! https://codereview.chromium.org/2963573004/diff/40001/components/browser_watc... File components/browser_watcher/watcher_metrics_provider_win_unittest.cc (right): https://codereview.chromium.org/2963573004/diff/40001/components/browser_watc... components/browser_watcher/watcher_metrics_provider_win_unittest.cc:139: key.Open(HKEY_CURRENT_USER, kRegistryPath, KEY_READ)); On 2017/07/04 12:26:25, manzagop (departed) wrote: > On 2017/07/03 02:35:07, Ilya Sherman wrote: > > On 2017/06/30 20:11:43, manzagop (departed) wrote: > > > On 2017/06/30 18:29:41, Ilya Sherman wrote: > > > > It's not clear to me whether this is appropriate to test with the task > > > > scheduler, nor that this works correctly in the task scheduler world, as > the > > > > code might run in parallel. I'm inclined to simply remove this check; but > > we > > > > could also instead substitute a test-specific task runner, and check this > > > > property using that task runner (similar to how this test used to work). > > > WDYT? > > > > > > Could this simply go to l.127? > > > > It could, but it would be changing the meaning of the test. The comment > > indicates that this is currently testing that the provider schedules the work > on > > a background thread, rather than performing it synchronously. Is this > something > > that we still want to test? And if not, is there value in preserving this > check > > to test something different -- e.g. that the provider does delete the key? > > Ah, got it. So deletion is the intent and the posting on background is a proxy > for not holding up the rest too much? I don't feel strongly about testing the > later. Perhaps a simple comment at the posting site to mention the concern about > the task being expensive? Done.
The CQ bit was checked by isherman@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...
lgtm
The CQ bit was unchecked by isherman@chromium.org
The CQ bit was checked by isherman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org Link to the patchset: https://codereview.chromium.org/2963573004/#ps120001 (title: "Document that registry interactions can block, and adjust the unittest not to be flaky")
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by isherman@chromium.org
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by isherman@chromium.org
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by isherman@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": 120001, "attempt_start_ts": 1499412353949200, "parent_rev": "198800bd155327971b0b74819fac18703862698c", "commit_rev": "1caa0802385fff5ac048cfe1423c19ab3a72acdf"}
Message was sent while issue was closed.
Description was changed from ========== [Cleanup] Migrate the WatcherMetricsProvider to use the Task Scheduler. BUG=667892 TEST=none R=asvitkine@chromium.org ========== to ========== [Cleanup] Migrate the WatcherMetricsProvider to use the Task Scheduler. BUG=667892 TEST=none R=asvitkine@chromium.org Review-Url: https://codereview.chromium.org/2963573004 Cr-Commit-Position: refs/heads/master@{#484860} Committed: https://chromium.googlesource.com/chromium/src/+/1caa0802385fff5ac048cfe1423c... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/1caa0802385fff5ac048cfe1423c... |