Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2368193003:
[sensors] Introduce asynchronous way to create sensors. (Closed)
|
Created:
4 years, 2 months ago by maksims (do not use this acc) Modified:
4 years, 2 months ago CC:
chromium-reviews, riju_, darktears Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sensors] Introduce asynchronous way to create sensors.
As long as sensors implementation on Linux/ChromeOS will
be based on reading from files (sensors' files are located
in /sys/dev/iio/devices), asynchronous way of creating
sensors is introduced with this patch.
To be more specific, the asynchronous path is needed because
sensors' existence will be checked by blocking IO methods in
case of Linux/ChromeOS and in order to be able to say
whether a sensor exists or not, asynchronous path will be
used.
In case of Android and Mac, callbacks will be
called immediately as soon as a sensor manager in Android or
IOServiceGetMatchingService in Mac returns a sensor object
or nullptr.
How it works:
As soon as a request for a new sensor is received, callbacks
are stored into a vector which is pushed into a map with a
type key and the vector itself. If no callback has been
stored before, CreateSensorInternal() method is called.
Then:
*Linux/ChromeOS (under development): a new thread is
created which will handle concrete sensors creation.
*Android: when CreateSensorInternal() is called, java code
is called and everything is handled by the sensor manager,
which returns synchronously. Once sensor found, it is
returned. Otherwise, nullptr is returned.
*Mac (under development)
https://codereview.chromium.org/2332903002/: everything
is handled by IOServiceGetMatchingService, which returns
synchronously.
BUG=606766
Committed: https://crrev.com/85cc22d79580bc5ad58db03ff3e4fb8f9db539a6
Cr-Commit-Position: refs/heads/master@{#423127}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Mikhail's comments #Patch Set 3 : more comments #Patch Set 4 : Added a temporary dependency for dependent patch that is based on top of this cl. Reason: cannot ha… #
Total comments: 11
Patch Set 5 : Tim's comments #
Total comments: 6
Patch Set 6 : avoid potential future reentrancy problems #
Total comments: 3
Patch Set 7 : fix unittests #Patch Set 8 : PostTask in helper function #Patch Set 9 : typo #
Total comments: 1
Patch Set 10 : more typos #Patch Set 11 : A comment from Mikhail #Messages
Total messages: 91 (61 generated)
Description was changed from ========== [sensors] Introduce asynchronous way to create sensors. BUG= ========== to ========== [sensors] Introduce asynchronous way to create sensors. As long as sensors implementation on Linux/ChromeOS will be based on reading from files (sensors' files are located in /sys/dev/iio/devices), asynchronous way of creating sensors is introduced with this patch. To be more specific, the asynchronous path is needed because sensors' existence will be checked by blocking IO methods and in order to be able to say whether a sensor exists or not, asynchronous path will be used. In case of Android and Mac, a mojo callback will be called immediately as soon as a sensor manager in Android or IOServiceGetMatchingService in Mac returns a sensor object or nullptr. How it works: As soon as a request for a new sensor is received, callbacks are stored into a vector which is pushed into a map with a type key and the vector itself. If no callback has been stored before, CreateSensorInternal() method is called. Then: *Linux/ChromeOS (under development): a new thread is created which will handle concrete sensors creation. *Android: when CreateSensorInternal() is called, java code is called and everything is handled by the sensor manager, which returns synchronously. Once sensor found, it is returned. Otherwise, nullptr is returned. *Mac (under development) https://codereview.chromium.org/2332903002/: everything is handled by IOServiceGetMatchingService, which returns synchronously. BUG=606766 ==========
maksim.sisov@intel.com changed reviewers: + alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com
The CQ bit was checked by maksim.sisov@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: 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 maksim.sisov@intel.com to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take a look. Do you I need to add anybody else to review the patch?
Description was changed from ========== [sensors] Introduce asynchronous way to create sensors. As long as sensors implementation on Linux/ChromeOS will be based on reading from files (sensors' files are located in /sys/dev/iio/devices), asynchronous way of creating sensors is introduced with this patch. To be more specific, the asynchronous path is needed because sensors' existence will be checked by blocking IO methods and in order to be able to say whether a sensor exists or not, asynchronous path will be used. In case of Android and Mac, a mojo callback will be called immediately as soon as a sensor manager in Android or IOServiceGetMatchingService in Mac returns a sensor object or nullptr. How it works: As soon as a request for a new sensor is received, callbacks are stored into a vector which is pushed into a map with a type key and the vector itself. If no callback has been stored before, CreateSensorInternal() method is called. Then: *Linux/ChromeOS (under development): a new thread is created which will handle concrete sensors creation. *Android: when CreateSensorInternal() is called, java code is called and everything is handled by the sensor manager, which returns synchronously. Once sensor found, it is returned. Otherwise, nullptr is returned. *Mac (under development) https://codereview.chromium.org/2332903002/: everything is handled by IOServiceGetMatchingService, which returns synchronously. BUG=606766 ========== to ========== [sensors] Introduce asynchronous way to create sensors. As long as sensors implementation on Linux/ChromeOS will be based on reading from files (sensors' files are located in /sys/dev/iio/devices), asynchronous way of creating sensors is introduced with this patch. To be more specific, the asynchronous path is needed because sensors' existence will be checked by blocking IO methods and in order to be able to say whether a sensor exists or not, asynchronous path will be used. In case of Android and Mac, a callback will be called immediately as soon as a sensor manager in Android or IOServiceGetMatchingService in Mac returns a sensor object or nullptr. How it works: As soon as a request for a new sensor is received, callbacks are stored into a vector which is pushed into a map with a type key and the vector itself. If no callback has been stored before, CreateSensorInternal() method is called. Then: *Linux/ChromeOS (under development): a new thread is created which will handle concrete sensors creation. *Android: when CreateSensorInternal() is called, java code is called and everything is handled by the sensor manager, which returns synchronously. Once sensor found, it is returned. Otherwise, nullptr is returned. *Mac (under development) https://codereview.chromium.org/2332903002/: everything is handled by IOServiceGetMatchingService, which returns synchronously. BUG=606766 ==========
Description was changed from ========== [sensors] Introduce asynchronous way to create sensors. As long as sensors implementation on Linux/ChromeOS will be based on reading from files (sensors' files are located in /sys/dev/iio/devices), asynchronous way of creating sensors is introduced with this patch. To be more specific, the asynchronous path is needed because sensors' existence will be checked by blocking IO methods and in order to be able to say whether a sensor exists or not, asynchronous path will be used. In case of Android and Mac, a callback will be called immediately as soon as a sensor manager in Android or IOServiceGetMatchingService in Mac returns a sensor object or nullptr. How it works: As soon as a request for a new sensor is received, callbacks are stored into a vector which is pushed into a map with a type key and the vector itself. If no callback has been stored before, CreateSensorInternal() method is called. Then: *Linux/ChromeOS (under development): a new thread is created which will handle concrete sensors creation. *Android: when CreateSensorInternal() is called, java code is called and everything is handled by the sensor manager, which returns synchronously. Once sensor found, it is returned. Otherwise, nullptr is returned. *Mac (under development) https://codereview.chromium.org/2332903002/: everything is handled by IOServiceGetMatchingService, which returns synchronously. BUG=606766 ========== to ========== [sensors] Introduce asynchronous way to create sensors. As long as sensors implementation on Linux/ChromeOS will be based on reading from files (sensors' files are located in /sys/dev/iio/devices), asynchronous way of creating sensors is introduced with this patch. To be more specific, the asynchronous path is needed because sensors' existence will be checked by blocking IO methods and in order to be able to say whether a sensor exists or not, asynchronous path will be used. In case of Android and Mac, callbacks will be called immediately as soon as a sensor manager in Android or IOServiceGetMatchingService in Mac returns a sensor object or nullptr. How it works: As soon as a request for a new sensor is received, callbacks are stored into a vector which is pushed into a map with a type key and the vector itself. If no callback has been stored before, CreateSensorInternal() method is called. Then: *Linux/ChromeOS (under development): a new thread is created which will handle concrete sensors creation. *Android: when CreateSensorInternal() is called, java code is called and everything is handled by the sensor manager, which returns synchronously. Once sensor found, it is returned. Otherwise, nullptr is returned. *Mac (under development) https://codereview.chromium.org/2332903002/: everything is handled by IOServiceGetMatchingService, which returns synchronously. BUG=606766 ==========
Description was changed from ========== [sensors] Introduce asynchronous way to create sensors. As long as sensors implementation on Linux/ChromeOS will be based on reading from files (sensors' files are located in /sys/dev/iio/devices), asynchronous way of creating sensors is introduced with this patch. To be more specific, the asynchronous path is needed because sensors' existence will be checked by blocking IO methods and in order to be able to say whether a sensor exists or not, asynchronous path will be used. In case of Android and Mac, callbacks will be called immediately as soon as a sensor manager in Android or IOServiceGetMatchingService in Mac returns a sensor object or nullptr. How it works: As soon as a request for a new sensor is received, callbacks are stored into a vector which is pushed into a map with a type key and the vector itself. If no callback has been stored before, CreateSensorInternal() method is called. Then: *Linux/ChromeOS (under development): a new thread is created which will handle concrete sensors creation. *Android: when CreateSensorInternal() is called, java code is called and everything is handled by the sensor manager, which returns synchronously. Once sensor found, it is returned. Otherwise, nullptr is returned. *Mac (under development) https://codereview.chromium.org/2332903002/: everything is handled by IOServiceGetMatchingService, which returns synchronously. BUG=606766 ========== to ========== [sensors] Introduce asynchronous way to create sensors. As long as sensors implementation on Linux/ChromeOS will be based on reading from files (sensors' files are located in /sys/dev/iio/devices), asynchronous way of creating sensors is introduced with this patch. To be more specific, the asynchronous path is needed because sensors' existence will be checked by blocking IO methods in case of Linux/ChromeOS and in order to be able to say whether a sensor exists or not, asynchronous path will be used. In case of Android and Mac, callbacks will be called immediately as soon as a sensor manager in Android or IOServiceGetMatchingService in Mac returns a sensor object or nullptr. How it works: As soon as a request for a new sensor is received, callbacks are stored into a vector which is pushed into a map with a type key and the vector itself. If no callback has been stored before, CreateSensorInternal() method is called. Then: *Linux/ChromeOS (under development): a new thread is created which will handle concrete sensors creation. *Android: when CreateSensorInternal() is called, java code is called and everything is handled by the sensor manager, which returns synchronously. Once sensor found, it is returned. Otherwise, nullptr is returned. *Mac (under development) https://codereview.chromium.org/2332903002/: everything is handled by IOServiceGetMatchingService, which returns synchronously. BUG=606766 ==========
https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_base.cc:33: return; it has to run callback with nullptr, right? https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_base.cc:38: return; ditto https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_base.h (right): https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_base.h:57: std::map<mojom::SensorType, CallbackQueue> requests_map_; let's keep PlatformSensorProvider simple and move pending requests management to SensorProviderImpl https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/s... File device/generic_sensor/sensor_provider_impl.h (right): https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/s... device/generic_sensor/sensor_provider_impl.h:40: using CreateSensorCallback = we don't need it in this header.
mikhail.pozdnyakov@intel.com changed reviewers: + timvolodine@chromium.org
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 maksim.sisov@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...
https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_base.cc:33: return; On 2016/09/26 09:52:23, Mikhail wrote: > it has to run callback with nullptr, right? Done. https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_base.cc:38: return; On 2016/09/26 09:52:23, Mikhail wrote: > ditto Done. https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_base.h (right): https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_base.h:57: std::map<mojom::SensorType, CallbackQueue> requests_map_; On 2016/09/26 09:52:23, Mikhail wrote: > let's keep PlatformSensorProvider simple and move pending requests management to > SensorProviderImpl We cannot do so. SensorProviderImpl is not a singleton class. Thus, when one opens a new tab, new SensorProviderImpl is created and there are different maps of callbacks. Request for creating a sensor will be triggered again and again. What is more, SensorProviderImpl will have to have two different paths to create sensors. 1) If no sensor, save callbacks to map and trigger a provider to create sensors. On callback run origin callbacks. 2) If there was a sensor, directly establish a sensor and run callback. Thus, there will be more code. https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/s... File device/generic_sensor/sensor_provider_impl.h (right): https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/s... device/generic_sensor/sensor_provider_impl.h:40: using CreateSensorCallback = On 2016/09/26 09:52:23, Mikhail wrote: > we don't need it in this header. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_base.cc:44: DCHECK(!ContainsKey(requests_map_, type)); This DCHECK would never fail, right? Maybe it is better to check that there are no duplicate pending requests in the request queue.
some nits https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_base.cc:46: CallbackQueue pending_requests; nit: could be 'requests_map_[type] = CallbackQueue({callback});' https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_base.cc:106: if (it != requests_map_.end()) { it can never be 'it == requests_map_.end()', condition should be omitted. https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/s... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/s... device/generic_sensor/sensor_provider_impl.cc:35: : provider_(provider), weak_ptr_factory_(this) { nit: next line https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/s... device/generic_sensor/sensor_provider_impl.cc:64: void SensorProviderImpl::CreateSensor( worth renaming to 'SensorCreated'. it does not create sensor actually.
On 2016/09/27 10:17:25, maksims wrote: > On 2016/09/26 09:52:23, Mikhail wrote: > > let's keep PlatformSensorProvider simple and move pending requests management > to > > SensorProviderImpl > > We cannot do so. SensorProviderImpl is not a singleton class. Thus, when one > opens a new tab, new SensorProviderImpl is created and there are different maps > of callbacks. right, let's keep it like this. An alternate approach would be introducing observer interface for sensor provider, but not sure if it is more beneficial.
The CQ bit was checked by maksim.sisov@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 #3 (id:60001) has been deleted
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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 maksim.sisov@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...
https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_base.cc:44: DCHECK(!ContainsKey(requests_map_, type)); On 2016/09/27 11:27:01, shalamov wrote: > > This DCHECK would never fail, right? Maybe it is better to check that there are > no duplicate pending requests in the request queue. Done. https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_base.cc:46: CallbackQueue pending_requests; On 2016/09/27 11:29:19, Mikhail wrote: > nit: could be 'requests_map_[type] = CallbackQueue({callback});' Done. https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_base.cc:106: if (it != requests_map_.end()) { On 2016/09/27 11:29:19, Mikhail wrote: > it can never be 'it == requests_map_.end()', condition should be omitted. Right. Done! https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/s... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/s... device/generic_sensor/sensor_provider_impl.cc:64: void SensorProviderImpl::CreateSensor( On 2016/09/27 11:29:19, Mikhail wrote: > worth renaming to 'SensorCreated'. it does not create sensor actually. Done.
https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/s... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2368193003/diff/20001/device/generic_sensor/s... device/generic_sensor/sensor_provider_impl.cc:35: : provider_(provider), weak_ptr_factory_(this) { On 2016/09/27 11:29:19, Mikhail wrote: > nit: next line Should I really move to the next line? Git cl format makes it back like this.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
timvolodine@, can we ask you for your comments?
FYI: Alexis@ and Riju@
The CQ bit was checked by maksim.sisov@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...
generally more 'flexible' of course, however regarding implementation on Linux/ChromeOS if there is a dedicated thread anyway, is the asynchronous sensor creation really necessary? https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:46: it->second.push_back(callback); should we be worried about potential memory drain (and OOM) in case of very many 'CreateSensor' calls? https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:106: for (auto& cb : it->second) nit: better name for cb? is it 'callback'? https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.h (right): https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.h:20: base::Callback<void(scoped_refptr<PlatformSensor>)>; typedef here and re-use below in CallbackQueue and sensor_provider_impl.cc?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/27 19:39:29, timvolodine wrote: > generally more 'flexible' of course, however regarding implementation on > Linux/ChromeOS if there is a dedicated thread anyway, is the asynchronous sensor > creation really necessary? Yes, it is necessary, because it should be known in advance that sensor doesn't exist rather than starting to read and check then. The code for linux/cros is located here https://codereview.chromium.org/2370343002/ .
The CQ bit was checked by maksim.sisov@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...
https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:46: it->second.push_back(callback); On 2016/09/27 19:39:29, timvolodine wrote: > should we be worried about potential memory drain (and OOM) in case of very many > 'CreateSensor' calls? Actually, no. It takes 2-3 milliseconds for a callback to come from mojo IPC and ~300 microseconds to search for a sensor. https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:106: for (auto& cb : it->second) On 2016/09/27 19:39:29, timvolodine wrote: > nit: better name for cb? is it 'callback'? Done. https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.h (right): https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.h:20: base::Callback<void(scoped_refptr<PlatformSensor>)>; On 2016/09/27 19:39:29, timvolodine wrote: > typedef here and re-use below in CallbackQueue and sensor_provider_impl.cc? Why typedef? There is no need for compatibility with C. Basically, it is recommended to use "using" alias instead of "typedef". https://chromium-cpp.appspot.com/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:46: it->second.push_back(callback); On 2016/09/28 07:45:37, maksims wrote: > On 2016/09/27 19:39:29, timvolodine wrote: > > should we be worried about potential memory drain (and OOM) in case of very > many > > 'CreateSensor' calls? > > Actually, no. It takes 2-3 milliseconds for a callback to come from mojo IPC and > ~300 microseconds to search for a sensor. hmm, even when the initial sensor creation takes a long time and requests 'pile up' during that time? https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.h (right): https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.h:20: base::Callback<void(scoped_refptr<PlatformSensor>)>; On 2016/09/28 07:45:37, maksims wrote: > On 2016/09/27 19:39:29, timvolodine wrote: > > typedef here and re-use below in CallbackQueue and sensor_provider_impl.cc? > > Why typedef? There is no need for compatibility with C. > Basically, it is recommended to use "using" alias instead of "typedef". > https://chromium-cpp.appspot.com/ right sorry, basically I mean why not reuse the 'CreateSensorCallback' name in the derived definition of CallbackQueue and sensor_provider_impl.cc?
On 2016/09/27 20:21:15, maksims wrote: > On 2016/09/27 19:39:29, timvolodine wrote: > > generally more 'flexible' of course, however regarding implementation on > > Linux/ChromeOS if there is a dedicated thread anyway, is the asynchronous > sensor > > creation really necessary? > Yes, it is necessary, because it should be known in advance that sensor doesn't > exist rather than starting to read and check then. > > The code for linux/cros is located here > https://codereview.chromium.org/2370343002/ . I guess my point is that if you have a separate thread it could block for a bit without any issues anyway.. And the asynchronous handling is not really needed on Mac and Android.
On 2016/09/29 13:20:32, timvolodine wrote: > On 2016/09/27 20:21:15, maksims wrote: > > On 2016/09/27 19:39:29, timvolodine wrote: > > > generally more 'flexible' of course, however regarding implementation on > > > Linux/ChromeOS if there is a dedicated thread anyway, is the asynchronous > > sensor > > > creation really necessary? > > Yes, it is necessary, because it should be known in advance that sensor > doesn't > > exist rather than starting to read and check then. > > > > The code for linux/cros is located here > > https://codereview.chromium.org/2370343002/ . > > I guess my point is that if you have a separate thread it could block for a bit > without any issues anyway.. And the asynchronous handling is not really needed > on Mac and Android. Regarding Linux/CrOS - how can a sensor be returned if it has been created on a separate thread? Only through the callbacks. Regarding android and mac - I might haven't expressed myself cleanly: mac and android will create sensors synchronously. Instead of returning a scoped_refptr from CreateSensor, a callback will be run immediately without any hangs. Lines 58-73 - https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/...
https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:46: it->second.push_back(callback); On 2016/09/29 13:17:26, timvolodine wrote: > On 2016/09/28 07:45:37, maksims wrote: > > On 2016/09/27 19:39:29, timvolodine wrote: > > > should we be worried about potential memory drain (and OOM) in case of very > > many > > > 'CreateSensor' calls? > > > > Actually, no. It takes 2-3 milliseconds for a callback to come from mojo IPC > and > > ~300 microseconds to search for a sensor. > > hmm, even when the initial sensor creation takes a long time and requests 'pile > up' during that time? We have one proxy per frame. This scenario is unlikely. There should be a lot of frames in order to create a huge amount of callbacks. It will run out of memory due to a huge number of frames rather than callbacks. https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.h (right): https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.h:20: base::Callback<void(scoped_refptr<PlatformSensor>)>; On 2016/09/29 13:17:26, timvolodine wrote: > On 2016/09/28 07:45:37, maksims wrote: > > On 2016/09/27 19:39:29, timvolodine wrote: > > > typedef here and re-use below in CallbackQueue and sensor_provider_impl.cc? > > > > Why typedef? There is no need for compatibility with C. > > Basically, it is recommended to use "using" alias instead of "typedef". > > https://chromium-cpp.appspot.com/ > > right sorry, basically I mean why not reuse the 'CreateSensorCallback' name in > the derived definition of CallbackQueue and sensor_provider_impl.cc? Right. That's done.
On 2016/09/30 06:22:33, maksims wrote: > On 2016/09/29 13:20:32, timvolodine wrote: > > On 2016/09/27 20:21:15, maksims wrote: > > > On 2016/09/27 19:39:29, timvolodine wrote: > > > > generally more 'flexible' of course, however regarding implementation on > > > > Linux/ChromeOS if there is a dedicated thread anyway, is the asynchronous > > > sensor > > > > creation really necessary? > > > Yes, it is necessary, because it should be known in advance that sensor > > doesn't > > > exist rather than starting to read and check then. > > > > > > The code for linux/cros is located here > > > https://codereview.chromium.org/2370343002/ . > > > > I guess my point is that if you have a separate thread it could block for a > bit > > without any issues anyway.. And the asynchronous handling is not really needed > > on Mac and Android. > > Regarding Linux/CrOS - how can a sensor be returned if it has been created on a > separate thread? Only through the callbacks. > Regarding android and mac - I might haven't expressed myself cleanly: mac and > android will create sensors synchronously. Instead of returning a scoped_refptr > from CreateSensor, a callback will be run immediately without any hangs. Lines > 58-73 - > https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... technically you could also use shared memory directly from the thread, but I guess the implementation here is different..
https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2368193003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:46: it->second.push_back(callback); On 2016/09/30 06:23:06, maksims wrote: > On 2016/09/29 13:17:26, timvolodine wrote: > > On 2016/09/28 07:45:37, maksims wrote: > > > On 2016/09/27 19:39:29, timvolodine wrote: > > > > should we be worried about potential memory drain (and OOM) in case of > very > > > many > > > > 'CreateSensor' calls? > > > > > > Actually, no. It takes 2-3 milliseconds for a callback to come from mojo IPC > > and > > > ~300 microseconds to search for a sensor. > > > > hmm, even when the initial sensor creation takes a long time and requests > 'pile > > up' during that time? > > We have one proxy per frame. This scenario is unlikely. There should be a lot of > frames in order to create a huge amount of callbacks. It will run out of memory > due to a huge number of frames rather than callbacks. I see https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.h (right): https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.h:54: std::vector<base::Callback<void(scoped_refptr<PlatformSensor>)>>; std::vector<CreateSensorCallback> ? https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:57: GetBufferOffset(type), cb); you mentioned this is 'synchronous' on android/mac i.e. the callback is executed immediately, are there any potential re-entrancy issues, if for example I call GetSensor inside the GetSensorCallback?
https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:57: GetBufferOffset(type), cb); On 2016/09/30 18:50:48, timvolodine wrote: > you mentioned this is 'synchronous' on android/mac i.e. the callback is executed > immediately, are there any potential re-entrancy issues, if for example I call > GetSensor inside the GetSensorCallback? It's technically possible, but it would be considered as a programming error if one calls getSensor on the same process using the same sensor_provider_impl. Do you think this should be fixed on this level?
https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:57: GetBufferOffset(type), cb); On 2016/10/03 12:58:11, maksims wrote: > On 2016/09/30 18:50:48, timvolodine wrote: > > you mentioned this is 'synchronous' on android/mac i.e. the callback is > executed > > immediately, are there any potential re-entrancy issues, if for example I call > > GetSensor inside the GetSensorCallback? > > It's technically possible, but it would be considered as a programming error if > one calls getSensor on the same process using the same sensor_provider_impl. Do > you think this should be fixed on this level? I think this should be addressed to some extent to avoid potential future issues.. I would either use a PostTask (instead of immediate callback invocation) or maybe a dcheck with a comment to avoid reentrancy. But handling everything in async way seems the easiest approach (probably without much if any overhead).
The CQ bit was checked by maksim.sisov@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@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...
https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.h (right): https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.h:54: std::vector<base::Callback<void(scoped_refptr<PlatformSensor>)>>; On 2016/09/30 18:50:48, timvolodine wrote: > std::vector<CreateSensorCallback> ? Done. https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2368193003/diff/120001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:57: GetBufferOffset(type), cb); On 2016/10/03 17:44:12, timvolodine wrote: > On 2016/10/03 12:58:11, maksims wrote: > > On 2016/09/30 18:50:48, timvolodine wrote: > > > you mentioned this is 'synchronous' on android/mac i.e. the callback is > > executed > > > immediately, are there any potential re-entrancy issues, if for example I > call > > > GetSensor inside the GetSensorCallback? > > > > It's technically possible, but it would be considered as a programming error > if > > one calls getSensor on the same process using the same sensor_provider_impl. > Do > > you think this should be fixed on this level? > > I think this should be addressed to some extent to avoid potential future > issues.. I would either use a PostTask (instead of immediate callback > invocation) or maybe a dcheck with a comment to avoid reentrancy. But handling > everything in async way seems the easiest approach (probably without much if any > overhead). Sure. As per discussion with shalamov@, we had the same conclusion (I mean using PostTask to run the callback.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ok thanks! some comments below, otherwise lgtm https://codereview.chromium.org/2368193003/diff/140001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2368193003/diff/140001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:52: callback.Run(nullptr, nullptr); also posttask? https://codereview.chromium.org/2368193003/diff/140001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:78: callback.Run(nullptr, nullptr); and here? https://codereview.chromium.org/2368193003/diff/140001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:90: base::MessageLoop::current()->task_runner()->PostTask( maybe use a helper function for this because in multiple places.
The CQ bit was checked by maksim.sisov@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...
On 2016/10/04 18:39:16, timvolodine wrote: > ok thanks! some comments below, otherwise lgtm > > https://codereview.chromium.org/2368193003/diff/140001/device/generic_sensor/... > File device/generic_sensor/sensor_provider_impl.cc (right): > > https://codereview.chromium.org/2368193003/diff/140001/device/generic_sensor/... > device/generic_sensor/sensor_provider_impl.cc:52: callback.Run(nullptr, > nullptr); > also posttask? > > https://codereview.chromium.org/2368193003/diff/140001/device/generic_sensor/... > device/generic_sensor/sensor_provider_impl.cc:78: callback.Run(nullptr, > nullptr); > and here? > > https://codereview.chromium.org/2368193003/diff/140001/device/generic_sensor/... > device/generic_sensor/sensor_provider_impl.cc:90: > base::MessageLoop::current()->task_runner()->PostTask( > maybe use a helper function for this because in multiple places. Done!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by maksim.sisov@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by maksim.sisov@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...
https://codereview.chromium.org/2368193003/diff/200001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2368193003/diff/200001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:33: base::MessageLoop::current()->task_runner()->PostTask( pls change it to 'base::ThreadTaskRunnerHandle::Get()'
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/05 08:31:29, Mikhail wrote: > https://codereview.chromium.org/2368193003/diff/200001/device/generic_sensor/... > File device/generic_sensor/sensor_provider_impl.cc (right): > > https://codereview.chromium.org/2368193003/diff/200001/device/generic_sensor/... > device/generic_sensor/sensor_provider_impl.cc:33: > base::MessageLoop::current()->task_runner()->PostTask( > pls change it to 'base::ThreadTaskRunnerHandle::Get()' done!
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from mikhail.pozdnyakov@intel.com, timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/2368193003/#ps240001 (title: "A comment from Mikhail")
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 ========== [sensors] Introduce asynchronous way to create sensors. As long as sensors implementation on Linux/ChromeOS will be based on reading from files (sensors' files are located in /sys/dev/iio/devices), asynchronous way of creating sensors is introduced with this patch. To be more specific, the asynchronous path is needed because sensors' existence will be checked by blocking IO methods in case of Linux/ChromeOS and in order to be able to say whether a sensor exists or not, asynchronous path will be used. In case of Android and Mac, callbacks will be called immediately as soon as a sensor manager in Android or IOServiceGetMatchingService in Mac returns a sensor object or nullptr. How it works: As soon as a request for a new sensor is received, callbacks are stored into a vector which is pushed into a map with a type key and the vector itself. If no callback has been stored before, CreateSensorInternal() method is called. Then: *Linux/ChromeOS (under development): a new thread is created which will handle concrete sensors creation. *Android: when CreateSensorInternal() is called, java code is called and everything is handled by the sensor manager, which returns synchronously. Once sensor found, it is returned. Otherwise, nullptr is returned. *Mac (under development) https://codereview.chromium.org/2332903002/: everything is handled by IOServiceGetMatchingService, which returns synchronously. BUG=606766 ========== to ========== [sensors] Introduce asynchronous way to create sensors. As long as sensors implementation on Linux/ChromeOS will be based on reading from files (sensors' files are located in /sys/dev/iio/devices), asynchronous way of creating sensors is introduced with this patch. To be more specific, the asynchronous path is needed because sensors' existence will be checked by blocking IO methods in case of Linux/ChromeOS and in order to be able to say whether a sensor exists or not, asynchronous path will be used. In case of Android and Mac, callbacks will be called immediately as soon as a sensor manager in Android or IOServiceGetMatchingService in Mac returns a sensor object or nullptr. How it works: As soon as a request for a new sensor is received, callbacks are stored into a vector which is pushed into a map with a type key and the vector itself. If no callback has been stored before, CreateSensorInternal() method is called. Then: *Linux/ChromeOS (under development): a new thread is created which will handle concrete sensors creation. *Android: when CreateSensorInternal() is called, java code is called and everything is handled by the sensor manager, which returns synchronously. Once sensor found, it is returned. Otherwise, nullptr is returned. *Mac (under development) https://codereview.chromium.org/2332903002/: everything is handled by IOServiceGetMatchingService, which returns synchronously. BUG=606766 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [sensors] Introduce asynchronous way to create sensors. As long as sensors implementation on Linux/ChromeOS will be based on reading from files (sensors' files are located in /sys/dev/iio/devices), asynchronous way of creating sensors is introduced with this patch. To be more specific, the asynchronous path is needed because sensors' existence will be checked by blocking IO methods in case of Linux/ChromeOS and in order to be able to say whether a sensor exists or not, asynchronous path will be used. In case of Android and Mac, callbacks will be called immediately as soon as a sensor manager in Android or IOServiceGetMatchingService in Mac returns a sensor object or nullptr. How it works: As soon as a request for a new sensor is received, callbacks are stored into a vector which is pushed into a map with a type key and the vector itself. If no callback has been stored before, CreateSensorInternal() method is called. Then: *Linux/ChromeOS (under development): a new thread is created which will handle concrete sensors creation. *Android: when CreateSensorInternal() is called, java code is called and everything is handled by the sensor manager, which returns synchronously. Once sensor found, it is returned. Otherwise, nullptr is returned. *Mac (under development) https://codereview.chromium.org/2332903002/: everything is handled by IOServiceGetMatchingService, which returns synchronously. BUG=606766 ========== to ========== [sensors] Introduce asynchronous way to create sensors. As long as sensors implementation on Linux/ChromeOS will be based on reading from files (sensors' files are located in /sys/dev/iio/devices), asynchronous way of creating sensors is introduced with this patch. To be more specific, the asynchronous path is needed because sensors' existence will be checked by blocking IO methods in case of Linux/ChromeOS and in order to be able to say whether a sensor exists or not, asynchronous path will be used. In case of Android and Mac, callbacks will be called immediately as soon as a sensor manager in Android or IOServiceGetMatchingService in Mac returns a sensor object or nullptr. How it works: As soon as a request for a new sensor is received, callbacks are stored into a vector which is pushed into a map with a type key and the vector itself. If no callback has been stored before, CreateSensorInternal() method is called. Then: *Linux/ChromeOS (under development): a new thread is created which will handle concrete sensors creation. *Android: when CreateSensorInternal() is called, java code is called and everything is handled by the sensor manager, which returns synchronously. Once sensor found, it is returned. Otherwise, nullptr is returned. *Mac (under development) https://codereview.chromium.org/2332903002/: everything is handled by IOServiceGetMatchingService, which returns synchronously. BUG=606766 Committed: https://crrev.com/85cc22d79580bc5ad58db03ff3e4fb8f9db539a6 Cr-Commit-Position: refs/heads/master@{#423127} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/85cc22d79580bc5ad58db03ff3e4fb8f9db539a6 Cr-Commit-Position: refs/heads/master@{#423127} |
