Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2359493002:
Prevent redundant DoWorks due to canceled delayed tasks (v2) (Closed)
|
Created:
4 years, 3 months ago by alex clarke (OOO till 29th) Modified:
4 years, 3 months ago CC:
chromium-reviews, blink-reviews, scheduler-bugs_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent redundant DoWorks due to canceled delayed tasks (v2)
The TimeDomain now only knows about the first wake up per queue
and the system tries to only have one pending delayed DoWork
posted on the base runloop. Because of this any cancelled
delayed tasks get removed by TaskQueueImpl::WakeUpForDelayedWork
before a wakeup is requested.
TimeDomain has been changed to take advantage of this new
limitation and several redundant methods have now been removed.
The bug which caused the revert of V1 has been fixed, specifically
ThrottlingHelper::PumpThrottledTasks doesn't get broken by the
new assumptions since we're no longer using ClearExpiredWakeups.
BUG=638542, 605718
Committed: https://crrev.com/917d60230680d15af69e12a34a44bfd25ca4784c
Cr-Commit-Position: refs/heads/master@{#420707}
Patch Set 1 #Patch Set 2 : Try and fix ASAN issue #Patch Set 3 : Added a test and removed some code we probably don't need. #
Total comments: 8
Patch Set 4 : Changes for Sami #
Total comments: 8
Patch Set 5 : Changes for Alexander #Messages
Total messages: 62 (48 generated)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Description was changed from ========== Prevent redundant DoWorks due to canceled delayed tasks (v2) Prevent redundant DoWorks due to canceled delayed tasks (v2). The TimeDomain now only knows about the first wake up per queue and the system tries to only have one pending delayed DoWork posted on the base runloop. Because of this any cancelled delayed tasks get removed by TaskQueueImpl::WakeUpForDelayedWork before a wakeup is requested. TimeDomain has been changed to take advantage of this new limitation and several redundant methods have now been removed. The bug which caused the revert of V1 has been fixed, specifically ThrottlingHelper::PumpThrottledTasks doesn't get broken by the new assumptions since we're no longer using ClearExpiredWakeups. BUG=638542, 605718 ========== to ========== Prevent redundant DoWorks due to canceled delayed tasks (v2) The TimeDomain now only knows about the first wake up per queue and the system tries to only have one pending delayed DoWork posted on the base runloop. Because of this any cancelled delayed tasks get removed by TaskQueueImpl::WakeUpForDelayedWork before a wakeup is requested. TimeDomain has been changed to take advantage of this new limitation and several redundant methods have now been removed. The bug which caused the revert of V1 has been fixed, specifically ThrottlingHelper::PumpThrottledTasks doesn't get broken by the new assumptions since we're no longer using ClearExpiredWakeups. BUG=638542, 605718 ==========
alexclarke@chromium.org changed reviewers: + altimin@chromium.org, skyostil@chromium.org
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...)
The CQ bit was checked by alexclarke@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.
The CQ bit was checked by alexclarke@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.
Overall looks like a nice simplification. https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:223: if (pending_task.delayed_run_time == Looks like a use-after-move. https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/time_domain.cc (right): https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/time_domain.cc:87: if (delayed_wakeup_multimap_.empty() || nit: Feel free to ignore, but I'm wondering if this would read better like this: if (queue was found) // remove queue // RequestWakeup if delayed run time is the next one if (queue was found) // update run time else // insert queue I.e., basically to avoid repeating the RequestWakeup block. Maybe the iterator lifetimes make this tricky... https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/time_domain.h (right): https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/time_domain.h:94: // registered wakeup. ...any previously registered wakeup for |queue|? https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/time_domain.h:139: // This map make it easy to remove a queue from |delayed_wakeup_multimap_|. nit: makes
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:223: if (pending_task.delayed_run_time == On 2016/09/22 11:20:54, Sami wrote: > Looks like a use-after-move. Done. https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/time_domain.cc (right): https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/time_domain.cc:87: if (delayed_wakeup_multimap_.empty() || On 2016/09/22 11:20:54, Sami wrote: > nit: Feel free to ignore, but I'm wondering if this would read better like this: > > if (queue was found) > // remove queue > > // RequestWakeup if delayed run time is the next one > > if (queue was found) > // update run time > else > // insert queue > > I.e., basically to avoid repeating the RequestWakeup block. Maybe the iterator > lifetimes make this tricky... Done. https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/time_domain.h (right): https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/time_domain.h:94: // registered wakeup. On 2016/09/22 11:20:54, Sami wrote: > ...any previously registered wakeup for |queue|? Done. https://codereview.chromium.org/2359493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/time_domain.h:139: // This map make it easy to remove a queue from |delayed_wakeup_multimap_|. On 2016/09/22 11:20:54, Sami wrote: > nit: makes Done.
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 alexclarke@chromium.org
The CQ bit was unchecked by alexclarke@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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by alexclarke@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...
non-owner lgtm with a couple of nits. https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:251: if (delayed_run_time < time_domain_now) { nit: less-or-equal maybe? https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc:1871: void NopTask(std::vector<base::TimeTicks>* run_times) { nit: Fix the name of the method, maybe? https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/time_domain.cc (right): https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/time_domain.cc:96: std::make_pair(delayed_run_time, queue)); nit: std::make_pair can be replaced with initializer list (if you prefer). https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/time_domain.h (right): https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/time_domain.h:143: std::map<internal::TaskQueueImpl*, DelayedWakeupMultimap::iterator>; nit: unordered_map?
Thank you, lgtm.
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 alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, altimin@chromium.org Link to the patchset: https://codereview.chromium.org/2359493002/#ps80001 (title: "Changes for Alexander")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:251: if (delayed_run_time < time_domain_now) { On 2016/09/23 10:04:15, altimin wrote: > nit: less-or-equal maybe? Done. https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc:1871: void NopTask(std::vector<base::TimeTicks>* run_times) { On 2016/09/23 10:04:15, altimin wrote: > nit: Fix the name of the method, maybe? Done. https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/time_domain.cc (right): https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/time_domain.cc:96: std::make_pair(delayed_run_time, queue)); On 2016/09/23 10:04:15, altimin wrote: > nit: std::make_pair can be replaced with initializer list (if you prefer). I'd quite like to use emplace here, but apparently the library we're using doesn't support it :< Anyway done. https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/time_domain.h (right): https://codereview.chromium.org/2359493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/time_domain.h:143: std::map<internal::TaskQueueImpl*, DelayedWakeupMultimap::iterator>; On 2016/09/23 10:04:15, altimin wrote: > nit: unordered_map? Yeah probably a small win, I expect boost::flat_map would work better here once that's allowed.
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 alexclarke@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 alexclarke@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_...)
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by alexclarke@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...)
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, altimin@chromium.org Link to the patchset: https://codereview.chromium.org/2359493002/#ps80001 (title: "Changes for Alexander")
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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by alexclarke@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 ========== Prevent redundant DoWorks due to canceled delayed tasks (v2) The TimeDomain now only knows about the first wake up per queue and the system tries to only have one pending delayed DoWork posted on the base runloop. Because of this any cancelled delayed tasks get removed by TaskQueueImpl::WakeUpForDelayedWork before a wakeup is requested. TimeDomain has been changed to take advantage of this new limitation and several redundant methods have now been removed. The bug which caused the revert of V1 has been fixed, specifically ThrottlingHelper::PumpThrottledTasks doesn't get broken by the new assumptions since we're no longer using ClearExpiredWakeups. BUG=638542, 605718 ========== to ========== Prevent redundant DoWorks due to canceled delayed tasks (v2) The TimeDomain now only knows about the first wake up per queue and the system tries to only have one pending delayed DoWork posted on the base runloop. Because of this any cancelled delayed tasks get removed by TaskQueueImpl::WakeUpForDelayedWork before a wakeup is requested. TimeDomain has been changed to take advantage of this new limitation and several redundant methods have now been removed. The bug which caused the revert of V1 has been fixed, specifically ThrottlingHelper::PumpThrottledTasks doesn't get broken by the new assumptions since we're no longer using ClearExpiredWakeups. BUG=638542, 605718 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Prevent redundant DoWorks due to canceled delayed tasks (v2) The TimeDomain now only knows about the first wake up per queue and the system tries to only have one pending delayed DoWork posted on the base runloop. Because of this any cancelled delayed tasks get removed by TaskQueueImpl::WakeUpForDelayedWork before a wakeup is requested. TimeDomain has been changed to take advantage of this new limitation and several redundant methods have now been removed. The bug which caused the revert of V1 has been fixed, specifically ThrottlingHelper::PumpThrottledTasks doesn't get broken by the new assumptions since we're no longer using ClearExpiredWakeups. BUG=638542, 605718 ========== to ========== Prevent redundant DoWorks due to canceled delayed tasks (v2) The TimeDomain now only knows about the first wake up per queue and the system tries to only have one pending delayed DoWork posted on the base runloop. Because of this any cancelled delayed tasks get removed by TaskQueueImpl::WakeUpForDelayedWork before a wakeup is requested. TimeDomain has been changed to take advantage of this new limitation and several redundant methods have now been removed. The bug which caused the revert of V1 has been fixed, specifically ThrottlingHelper::PumpThrottledTasks doesn't get broken by the new assumptions since we're no longer using ClearExpiredWakeups. BUG=638542, 605718 Committed: https://crrev.com/917d60230680d15af69e12a34a44bfd25ca4784c Cr-Commit-Position: refs/heads/master@{#420707} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/917d60230680d15af69e12a34a44bfd25ca4784c Cr-Commit-Position: refs/heads/master@{#420707} |
