Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503853002:
[Sensors] Improvements in fetching reading for sensors with continuous reporting mode (Closed)
|
Created:
4 years, 1 month ago by Mikhail Modified:
3 years, 12 months ago Reviewers:
Reilly Grant (use Gerrit), Sami, shalamov, timvolodine, haraken, dtapuska, tdresser, alex clarke (OOO till 29th) CC:
chromium-reviews, blink-reviews, haraken Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sensors] Improvements in fetching reading for sensors with continuous reporting mode
If a platform sensors has continuous reporting mode its updates are not send via IPC, so there is a timer on Blink side which periodically reads the sensor data from shared buffer.
Before this change every sensor instance had its own repeating timer, however in case of multiple sensor instances it could bloat the thread task queue with unneeded requests to read the sensor data from shared buffer.
This patch introduces only one repeating timer per 'SensorProxy' (i.e. per frame) which is shared between all sensor instances of the same type.
Another consequent change is that now the 'SensorProxy' class is inherited from 'PageVisibilityObserver' (and the 'Sensor' class is not any more) which makes suspend/resume code path more efficient.
BUG=606766
Committed: https://crrev.com/7ef84cca9ddf9928a08967084030632d21b7fbff
Cr-Commit-Position: refs/heads/master@{#434133}
Patch Set 1 #Patch Set 2 : Fixed assertion hit #
Total comments: 8
Patch Set 3 : Comments from haraken@ + rename m_sensors -> m_sensorProxies #
Total comments: 1
Patch Set 4 : Comments from Alex #
Total comments: 8
Patch Set 5 : Comments from Tim #Messages
Total messages: 51 (31 generated)
The CQ bit was checked by mikhail.pozdnyakov@intel.com 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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by mikhail.pozdnyakov@intel.com 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...
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Sensors] Improvements in fetching reading for sensors with continuous reporting mode If a platform sensors has continuous reporting mode its updates are not send via IPC, so there is a timer on Blink side which periodically reads the sensor data from shared buffer. Before this change every sensor instance had its own repeating timer, however in case of multiple sensor instances it could bloat the thread task queue with unneeded requests to read the sensor data from shared buffer. This patch introduces only one repeating timer per 'SensorProxy' (i.e. per frame) which is shared between all sensor instances of the same type. Another consequent change is that now the 'SensorProxy' class is inherited from 'PageVisibilityObserver' (and the 'Sensor' class is not any more) which makes suspend/resume code path more efficient. BUG=606766 ========== to ========== [Sensors] Improvements in fetching reading for sensors with continuous reporting mode If a platform sensors has continuous reporting mode its updates are not send via IPC, so there is a timer on Blink side which periodically reads the sensor data from shared buffer. Before this change every sensor instance had its own repeating timer, however in case of multiple sensor instances it could bloat the thread task queue with unneeded requests to read the sensor data from shared buffer. This patch introduces only one repeating timer per 'SensorProxy' (i.e. per frame) which is shared between all sensor instances of the same type. Another consequent change is that now the 'SensorProxy' class is inherited from 'PageVisibilityObserver' (and the 'Sensor' class is not any more) which makes suspend/resume code path more efficient. BUG=606766 ==========
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 mikhail.pozdnyakov@intel.com 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.
Please take a look
https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:290: double repeatInterval = 1 / *it; does this mean that if I register a sensor with a low frequency first somebody else in the frame can make it fire changes with a higher frequency?
https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:290: double repeatInterval = 1 / *it; On 2016/11/15 18:27:56, timvolodine wrote: > does this mean that if I register a sensor with a low frequency first somebody > else in the frame can make it fire changes with a higher frequency? not actually, the given frequency will remain for every sensor instance (pls. see https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour...)
https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:167: Page* page = toDocument(getExecutionContext())->page(); toDocument(getExecutionContext()) => document https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp (right): https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp:59: m_timer.startOneShot(waitingTime, BLINK_FROM_HERE); In general, it's discouraged to trigger timers periodically because it wakes up the blink scheduler and wastes battery. Would it be an option to use an idle task for this?
Thanks for review! https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:167: Page* page = toDocument(getExecutionContext())->page(); On 2016/11/16 04:31:47, haraken wrote: > > toDocument(getExecutionContext()) => document Done. https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp (right): https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp:59: m_timer.startOneShot(waitingTime, BLINK_FROM_HERE); On 2016/11/16 04:31:47, haraken wrote: > > In general, it's discouraged to trigger timers periodically because it wakes up > the blink scheduler and wastes battery. > > Would it be an option to use an idle task for this? Sensor API clients might want to receive data in timely manner, especially from those sensors with continuous reporting mode (for example from motion sensors) to provide better UX, so IMO it does not look like a low-priority background service and idle task would not be quite appropriate.
haraken@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
Either way, this CL is not changing the existing timer behavior, so this CL LGTM on my side. https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp (right): https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp:59: m_timer.startOneShot(waitingTime, BLINK_FROM_HERE); On 2016/11/17 17:07:35, Mikhail wrote: > On 2016/11/16 04:31:47, haraken wrote: > > > > In general, it's discouraged to trigger timers periodically because it wakes > up > > the blink scheduler and wastes battery. > > > > Would it be an option to use an idle task for this? > > Sensor API clients might want to receive data in timely manner, especially from > those sensors with continuous reporting mode (for example from motion sensors) > to provide better UX, so IMO it does not look like a low-priority background > service and idle task would not be quite appropriate. +Sami +Alex
The CQ bit was checked by mikhail.pozdnyakov@intel.com 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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2503853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2503853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/Sensor.cpp:287: updateState(Sensor::SensorState::Errored); Maybe make helper function with: if (m_sensorUpdateNotifier) m_sensorUpdateNotifier->cancelPendingNotifications(); And then use it from reportError, onSuspended and stopListening?
lgtm
skyostil@chromium.org changed reviewers: + tdresser@chromium.org
https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp (right): https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp:59: m_timer.startOneShot(waitingTime, BLINK_FROM_HERE); On 2016/11/17 18:30:58, haraken wrote: > On 2016/11/17 17:07:35, Mikhail wrote: > > On 2016/11/16 04:31:47, haraken wrote: > > > > > > In general, it's discouraged to trigger timers periodically because it wakes > > up > > > the blink scheduler and wastes battery. > > > > > > Would it be an option to use an idle task for this? > > > > Sensor API clients might want to receive data in timely manner, especially > from > > those sensors with continuous reporting mode (for example from motion sensors) > > to provide better UX, so IMO it does not look like a low-priority background > > service and idle task would not be quite appropriate. > > +Sami +Alex In the future it might be more appropriate to treat sensor data like user input (+Tim) and schedule it together with rendering. By the way, looks like we could you startRepeating() instead of startOneShot() here, right? That would simplify the code and get you drift correction as a bonus.
On 2016/11/18 19:25:18, Sami (slow -- traveling) wrote: > > By the way, looks like we could you startRepeating() instead of startOneShot() > here, right? That would simplify the code and get you drift correction as a > bonus. That is true for sensors with continuous reporting mode, however for 'onchange' sensors (when data from platform comes at arbitrary moments) we'll still need single shots.
Reilly, Tim, Sami, do you have any more comments on this CL?
lgtm from my side. Maybe add a TODO about using repeating timers when it makes sense, but I'll leave that up to you.
Just a few comments, otherwise looks fine to me. Also, probably best to format the description to a fixed width I think we usually use 70/80 chars don't remember. Otherwise it ends up less readable when looking at logs in a terminal. https://codereview.chromium.org/2503853002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2503853002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:286: auto it = nit: maybe better to use a double instead of auto to be sure there is no rounding when computing repeatInterval later https://codereview.chromium.org/2503853002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:287: std::max_element(m_frequenciesUsed.begin(), m_frequenciesUsed.end()); nit: I guess you could use a priority queue (max heap) to avoid iterating through all of them. Not sure if it makes a difference here though.. (maybe a TODO?) https://codereview.chromium.org/2503853002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp (right): https://codereview.chromium.org/2503853002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp:44: return; // Skipping changes if update notifiation was already sheduled. ->notification https://codereview.chromium.org/2503853002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp:59: m_timer.startOneShot(waitingTime, BLINK_FROM_HERE); so there is still a timer for each sensor? I understand if there is a high frequency sensor active and this is a slow one, it would throw away most onSensorReadingChanged notifications and occasionally schedule its own "oneshot" timer?
On 2016/11/18 19:25:18, Sami wrote: > https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp > (right): > > https://codereview.chromium.org/2503853002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp:59: > m_timer.startOneShot(waitingTime, BLINK_FROM_HERE); > On 2016/11/17 18:30:58, haraken wrote: > > On 2016/11/17 17:07:35, Mikhail wrote: > > > On 2016/11/16 04:31:47, haraken wrote: > > > > > > > > In general, it's discouraged to trigger timers periodically because it > wakes > > > up > > > > the blink scheduler and wastes battery. > > > > > > > > Would it be an option to use an idle task for this? > > > > > > Sensor API clients might want to receive data in timely manner, especially > > from > > > those sensors with continuous reporting mode (for example from motion > sensors) > > > to provide better UX, so IMO it does not look like a low-priority background > > > service and idle task would not be quite appropriate. > > > > +Sami +Alex > > In the future it might be more appropriate to treat sensor data like user input > (+Tim) and schedule it together with rendering. yes indeed it would be great to sync the real-time sensors with rendering e.g. this is particularly helpful for VR/AR when tracking motion
The CQ bit was checked by mikhail.pozdnyakov@intel.com 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...
Thanks for your comments! https://codereview.chromium.org/2503853002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right): https://codereview.chromium.org/2503853002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:286: auto it = On 2016/11/21 16:30:35, timvolodine wrote: > nit: maybe better to use a double instead of auto to be sure there is no > rounding when computing repeatInterval later the returned type is actually 'std::vector<double>::iterator', so using 'auto' should not cause rounding issues, I would prefer keeping it. https://codereview.chromium.org/2503853002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:287: std::max_element(m_frequenciesUsed.begin(), m_frequenciesUsed.end()); On 2016/11/21 16:30:35, timvolodine wrote: > nit: I guess you could use a priority queue (max heap) to avoid iterating > through all of them. Not sure if it makes a difference here though.. (maybe a > TODO?) Done. https://codereview.chromium.org/2503853002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp (right): https://codereview.chromium.org/2503853002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp:44: return; // Skipping changes if update notifiation was already sheduled. On 2016/11/21 16:30:35, timvolodine wrote: > ->notification Done. https://codereview.chromium.org/2503853002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp:59: m_timer.startOneShot(waitingTime, BLINK_FROM_HERE); On 2016/11/21 16:30:35, timvolodine wrote: > so there is still a timer for each sensor? yes, but it is not repeating, single shot is scheduled only when needed (actual data changed). > > I understand if there is a high frequency sensor active and this is a slow one, > it would throw away most onSensorReadingChanged notifications and occasionally > schedule its own "oneshot" timer? yes, this is the desired behavior.
tdresser@chromium.org changed reviewers: + dtapuska@chromium.org
+dtapuska@ I don't think we should be using a repeating timer here, we should synchronize the read with rAF, like we're doing for other continuous input. This is likely fine for now, but can you file a bug to use the rAF signal instead of a timer (unless there's a good reason not to that I'm not seeing)?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/22 13:24:39, tdresser wrote: > +dtapuska@ > > I don't think we should be using a repeating timer here, we should synchronize > the read with rAF, like we're doing for other continuous input. > > This is likely fine for now, but can you file a bug to use the rAF signal > instead of a timer (unless there's a good reason not to that I'm not seeing)? Done: https://bugs.chromium.org/p/chromium/issues/detail?id=668052
The CQ bit was checked by mikhail.pozdnyakov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, skyostil@chromium.org, alexander.shalamov@intel.com Link to the patchset: https://codereview.chromium.org/2503853002/#ps100001 (title: "Comments from Tim")
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": 100001, "attempt_start_ts": 1479889116287570,
"parent_rev": "a38857b83e88f5472b0d4858cae0a49e9ed1831f", "commit_rev":
"467e100ad6642c77523621fa008b96b9c3e9cd66"}
Message was sent while issue was closed.
Description was changed from ========== [Sensors] Improvements in fetching reading for sensors with continuous reporting mode If a platform sensors has continuous reporting mode its updates are not send via IPC, so there is a timer on Blink side which periodically reads the sensor data from shared buffer. Before this change every sensor instance had its own repeating timer, however in case of multiple sensor instances it could bloat the thread task queue with unneeded requests to read the sensor data from shared buffer. This patch introduces only one repeating timer per 'SensorProxy' (i.e. per frame) which is shared between all sensor instances of the same type. Another consequent change is that now the 'SensorProxy' class is inherited from 'PageVisibilityObserver' (and the 'Sensor' class is not any more) which makes suspend/resume code path more efficient. BUG=606766 ========== to ========== [Sensors] Improvements in fetching reading for sensors with continuous reporting mode If a platform sensors has continuous reporting mode its updates are not send via IPC, so there is a timer on Blink side which periodically reads the sensor data from shared buffer. Before this change every sensor instance had its own repeating timer, however in case of multiple sensor instances it could bloat the thread task queue with unneeded requests to read the sensor data from shared buffer. This patch introduces only one repeating timer per 'SensorProxy' (i.e. per frame) which is shared between all sensor instances of the same type. Another consequent change is that now the 'SensorProxy' class is inherited from 'PageVisibilityObserver' (and the 'Sensor' class is not any more) which makes suspend/resume code path more efficient. BUG=606766 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Sensors] Improvements in fetching reading for sensors with continuous reporting mode If a platform sensors has continuous reporting mode its updates are not send via IPC, so there is a timer on Blink side which periodically reads the sensor data from shared buffer. Before this change every sensor instance had its own repeating timer, however in case of multiple sensor instances it could bloat the thread task queue with unneeded requests to read the sensor data from shared buffer. This patch introduces only one repeating timer per 'SensorProxy' (i.e. per frame) which is shared between all sensor instances of the same type. Another consequent change is that now the 'SensorProxy' class is inherited from 'PageVisibilityObserver' (and the 'Sensor' class is not any more) which makes suspend/resume code path more efficient. BUG=606766 ========== to ========== [Sensors] Improvements in fetching reading for sensors with continuous reporting mode If a platform sensors has continuous reporting mode its updates are not send via IPC, so there is a timer on Blink side which periodically reads the sensor data from shared buffer. Before this change every sensor instance had its own repeating timer, however in case of multiple sensor instances it could bloat the thread task queue with unneeded requests to read the sensor data from shared buffer. This patch introduces only one repeating timer per 'SensorProxy' (i.e. per frame) which is shared between all sensor instances of the same type. Another consequent change is that now the 'SensorProxy' class is inherited from 'PageVisibilityObserver' (and the 'Sensor' class is not any more) which makes suspend/resume code path more efficient. BUG=606766 Committed: https://crrev.com/7ef84cca9ddf9928a08967084030632d21b7fbff Cr-Commit-Position: refs/heads/master@{#434133} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7ef84cca9ddf9928a08967084030632d21b7fbff Cr-Commit-Position: refs/heads/master@{#434133} |
