Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695223008:
[Chromecast] Add an alarm manager for firing events on wall clock time. (Closed)
Description[Chromecast] Add an alarm manager for firing events on wall clock time.
PostDelayedTask is subject to time changes, clock drifts, and other factors.
Long duration tasks are even more sensitive.
Polling the wall clock every 5 seconds will allow execution close to the
requested time. If the process is suspended, the task will fire within
5 seconds of being woken up.
BUG= internal b/33114736
Review-Url: https://codereview.chromium.org/2695223008
Cr-Commit-Position: refs/heads/master@{#452335}
Committed: https://chromium.googlesource.com/chromium/src/+/3aeb76a311e61823dd4d6602bd0c213be085aba9
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addresses comments #
Total comments: 10
Patch Set 3 : Fixed nits #Patch Set 4 : Use caller's thread #
Total comments: 4
Patch Set 5 : Fixed nits #
Messages
Total messages: 21 (10 generated)
Description was changed from ========== [Chromecast] Add an alarm manager for firing events on wall clock time. PostDelayedTask is subject to time changes, clock drifts, and other factors. Long duration tasks are even more sensitive. Polling the wall clock every 5 seconds will allow execution close to the requested time. If the process is suspended, the task will fire within 5 seconds of being woken up. BUG= internal b/33114736 ========== to ========== [Chromecast] Add an alarm manager for firing events on wall clock time. PostDelayedTask is subject to time changes, clock drifts, and other factors. Long duration tasks are even more sensitive. Polling the wall clock every 5 seconds will allow execution close to the requested time. If the process is suspended, the task will fire within 5 seconds of being woken up. BUG= internal b/33114736 ==========
ryanchung@chromium.org changed reviewers: + dougsteed@chromium.org, halliwell@chromium.org
Please take a look. The overall goal is so that we can execute some tasks as soon as possible past a particular time. For refreshing things that expire at a particular wall clock time, this should provide more accuracy.
https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... File chromecast/base/alarm_manager.h (right): https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:38: class AlarmDelegate { I wonder if it would be less intrusive for the caller to pass a Callback? No need to implement Delegate interface, they can just bind whatever function they want. It might also allow easier lifetime management for clients (e.g. could bind with weak ptr and no need to RemoveAlarm). https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:47: // Construct and start the alarm manager. Is this intended to be some kind of singleton? Where will it be instantiated and accessed from? Given that we probably don't anticipate super widespread usage (is my assumption right?), I wonder if a cleaner design would be to make something embeddable in a class to provide an alarm for just that class. The downside is that if we had a lot of alarm clients, it would end up with lots of clock polling. But if we anticipate relatively few users, there are benefits ... * No need for clumsiness of singleton or having to pass a central manager instance around to where it's needed * Fewer/no threading concerns * Simpler implementation (e.g. no need for priority queue, locks)? https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:74: typedef std::map<AlarmDelegate*, std::unique_ptr<AlarmInfo>> AlarmMap; nit: 'using' preferred over typedef now. https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:79: void Start(); nit: seems like 'Start' code could be inlined in ctor (use delegating ctor for one form to call the other) https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:81: void Stop(); nit: could inline this code in dtor. https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:83: base::Lock alarms_lock_; Is there a strong reason to use a lock (vs posting tasks to add alarms on the same thread where CheckAlarm runs)? https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:99: } // chromecast nit: "namespace chromecast"
https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... File chromecast/base/alarm_manager.h (right): https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:38: class AlarmDelegate { On 2017/02/20 22:28:26, halliwell wrote: > I wonder if it would be less intrusive for the caller to pass a Callback? No > need to implement Delegate interface, they can just bind whatever function they > want. It might also allow easier lifetime management for clients (e.g. could > bind with weak ptr and no need to RemoveAlarm). Done. I'm wondering if there's a need to explicitly cancel an alarm without destroying the client. sanfin@ suggested returning a handle that must be kept alive for each alarm. That would allow the client to cancel alarms without killing itself. However, now that adding an alarm involves posting a task to the alarm manager, it's a little more complicated to implement that. What do you think? https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:47: // Construct and start the alarm manager. On 2017/02/20 22:28:26, halliwell wrote: > Is this intended to be some kind of singleton? Where will it be instantiated > and accessed from? > Given that we probably don't anticipate super widespread usage (is my assumption > right?), I wonder if a cleaner design would be to make something embeddable in a > class to provide an alarm for just that class. The downside is that if we had a > lot of alarm clients, it would end up with lots of clock polling. But if we > anticipate relatively few users, there are benefits ... > * No need for clumsiness of singleton or having to pass a central manager > instance around to where it's needed > * Fewer/no threading concerns > * Simpler implementation (e.g. no need for priority queue, locks)? Yes, this will be some sort of singleton instantiated and owned by CCService. A quick brainstorm with sanfin@ identified 3 places this would be used initially (TLS cert update, CRL download, and daily reboot). I agree having separate alarms in each class would be cleaner but I'm concerned that the number of clock polling threads could grow unchecked if usage increases. I'm not sure how many threads/polls-per-second would have effects on our performance. But at that point, is it more beneficial to poll more often on a single poller to get more accuracy? (vs having multiple separate pollers that poll less often?) If the implementation here is too risky, we can go with the separate pollers. https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:74: typedef std::map<AlarmDelegate*, std::unique_ptr<AlarmInfo>> AlarmMap; On 2017/02/20 22:28:26, halliwell wrote: > nit: 'using' preferred over typedef now. Done. https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:79: void Start(); On 2017/02/20 22:28:26, halliwell wrote: > nit: seems like 'Start' code could be inlined in ctor (use delegating ctor for > one form to call the other) Done. https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:81: void Stop(); On 2017/02/20 22:28:26, halliwell wrote: > nit: could inline this code in dtor. Done. https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:83: base::Lock alarms_lock_; On 2017/02/20 22:28:26, halliwell wrote: > Is there a strong reason to use a lock (vs posting tasks to add alarms on the > same thread where CheckAlarm runs)? Done. Posting is better. Updated code. https://codereview.chromium.org/2695223008/diff/1/chromecast/base/alarm_manag... chromecast/base/alarm_manager.h:99: } // chromecast On 2017/02/20 22:28:26, halliwell wrote: > nit: "namespace chromecast" Done.
Ok, if instantiating/accessing the object isn't onerous, sounds ok to keep a single one. https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... File chromecast/base/alarm_manager.cc (right): https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... chromecast/base/alarm_manager.cc:23: int kClockPollInterval = 5; nit: kClockPollIntervalSeconds. Comment probably unnecessary :) https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... chromecast/base/alarm_manager.cc:39: alarm_thread_.reset(new base::Thread("Cast_Alarm_Manager_Thread")); hmmm ... why do we need to create a thread specially for this? Seems like we should always use an existing one? https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... File chromecast/base/alarm_manager.h (right): https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... chromecast/base/alarm_manager.h:8: #include <map> unused? https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... chromecast/base/alarm_manager.h:18: #include "base/timer/timer.h" nit, can just forward-declare for things held in unique_ptr
https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... File chromecast/base/alarm_manager.cc (right): https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... chromecast/base/alarm_manager.cc:23: int kClockPollInterval = 5; On 2017/02/22 19:09:15, halliwell wrote: > nit: kClockPollIntervalSeconds. Comment probably unnecessary :) Done. heh https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... chromecast/base/alarm_manager.cc:39: alarm_thread_.reset(new base::Thread("Cast_Alarm_Manager_Thread")); On 2017/02/22 19:09:15, halliwell wrote: > hmmm ... why do we need to create a thread specially for this? Seems like we > should always use an existing one? Should we use the UI thread for this? At initialization time in CCService, the UI thread seems to be the only thread that makes sense. I wasn't sure if this kind of task should be kept out of the UI thread. https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... File chromecast/base/alarm_manager.h (right): https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... chromecast/base/alarm_manager.h:8: #include <map> On 2017/02/22 19:09:15, halliwell wrote: > unused? Done. https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... chromecast/base/alarm_manager.h:18: #include "base/timer/timer.h" On 2017/02/22 19:09:15, halliwell wrote: > nit, can just forward-declare for things held in unique_ptr Done. thx.
https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... File chromecast/base/alarm_manager.cc (right): https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... chromecast/base/alarm_manager.cc:39: alarm_thread_.reset(new base::Thread("Cast_Alarm_Manager_Thread")); On 2017/02/22 19:39:39, ryanchung wrote: > On 2017/02/22 19:09:15, halliwell wrote: > > hmmm ... why do we need to create a thread specially for this? Seems like we > > should always use an existing one? > > Should we use the UI thread for this? At initialization time in CCService, the > UI thread seems to be the only thread that makes sense. I wasn't sure if this > kind of task should be kept out of the UI thread. Yep, should be fine ... doesn't look like you're doing anything heavy here.
https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... File chromecast/base/alarm_manager.cc (right): https://codereview.chromium.org/2695223008/diff/20001/chromecast/base/alarm_m... chromecast/base/alarm_manager.cc:39: alarm_thread_.reset(new base::Thread("Cast_Alarm_Manager_Thread")); On 2017/02/22 19:42:43, halliwell wrote: > On 2017/02/22 19:39:39, ryanchung wrote: > > On 2017/02/22 19:09:15, halliwell wrote: > > > hmmm ... why do we need to create a thread specially for this? Seems like > we > > > should always use an existing one? > > > > Should we use the UI thread for this? At initialization time in CCService, the > > UI thread seems to be the only thread that makes sense. I wasn't sure if this > > kind of task should be kept out of the UI thread. > > Yep, should be fine ... doesn't look like you're doing anything heavy here. Cool. Done. Using base::ThreadTaskRunnerHandle::Get() to use the thread from the owner of this.
lgtm % nits https://codereview.chromium.org/2695223008/diff/60001/chromecast/base/alarm_m... File chromecast/base/alarm_manager.cc (right): https://codereview.chromium.org/2695223008/diff/60001/chromecast/base/alarm_m... chromecast/base/alarm_manager.cc:53: clock_tick_timer_.reset(nullptr); nit: is this necessary? should be part of default destruction behaviour anyway? https://codereview.chromium.org/2695223008/diff/60001/chromecast/base/alarm_m... File chromecast/base/alarm_manager.h (right): https://codereview.chromium.org/2695223008/diff/60001/chromecast/base/alarm_m... chromecast/base/alarm_manager.h:32: // thread used to set the alarm. nit, I guess remove functionality is gone now. And it may be better to move some of these comments (e.g. about original thread) down to the member functions they're describing.
https://codereview.chromium.org/2695223008/diff/60001/chromecast/base/alarm_m... File chromecast/base/alarm_manager.cc (right): https://codereview.chromium.org/2695223008/diff/60001/chromecast/base/alarm_m... chromecast/base/alarm_manager.cc:53: clock_tick_timer_.reset(nullptr); On 2017/02/22 19:58:55, halliwell wrote: > nit: is this necessary? should be part of default destruction behaviour anyway? Done. https://codereview.chromium.org/2695223008/diff/60001/chromecast/base/alarm_m... File chromecast/base/alarm_manager.h (right): https://codereview.chromium.org/2695223008/diff/60001/chromecast/base/alarm_m... chromecast/base/alarm_manager.h:32: // thread used to set the alarm. On 2017/02/22 19:58:55, halliwell wrote: > nit, I guess remove functionality is gone now. And it may be better to move > some of these comments (e.g. about original thread) down to the member functions > they're describing. Done.
The CQ bit was checked by ryanchung@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 ryanchung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org Link to the patchset: https://codereview.chromium.org/2695223008/#ps80001 (title: "Fixed nits")
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": 1487813119482350,
"parent_rev": "c71ef8d720f0aea74a3ac4efe15df87670fa3875", "commit_rev":
"3aeb76a311e61823dd4d6602bd0c213be085aba9"}
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Add an alarm manager for firing events on wall clock time. PostDelayedTask is subject to time changes, clock drifts, and other factors. Long duration tasks are even more sensitive. Polling the wall clock every 5 seconds will allow execution close to the requested time. If the process is suspended, the task will fire within 5 seconds of being woken up. BUG= internal b/33114736 ========== to ========== [Chromecast] Add an alarm manager for firing events on wall clock time. PostDelayedTask is subject to time changes, clock drifts, and other factors. Long duration tasks are even more sensitive. Polling the wall clock every 5 seconds will allow execution close to the requested time. If the process is suspended, the task will fire within 5 seconds of being woken up. BUG= internal b/33114736 Review-Url: https://codereview.chromium.org/2695223008 Cr-Commit-Position: refs/heads/master@{#452335} Committed: https://chromium.googlesource.com/chromium/src/+/3aeb76a311e61823dd4d6602bd0c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3aeb76a311e61823dd4d6602bd0c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
