[Boot Performance] sensors_open() takes ~480 ms during boot

RESOLVED WONTFIX

Status

Firefox OS
Performance
P4
minor
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: cyu, Unassigned)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

2 years ago
During boot, we call sensors_open() on the main thread, which takes about 480 ms. We should move it to the polling thread if it's not required to be called on the main thread.

See the range [5400, 6000] ms of the profile: https://people.mozilla.org/~bgirard/cleopatra/#report=faa95f297fc5a1421c0c1d00e2e3dbb9ac79532c&filter=[{%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A2935,%22end%22%3A9869}]&selection=0,1,149,979,995
(Reporter)

Comment 1

2 years ago
Created attachment 8634651 [details]
MozReview Request: Bug 1184081 - Move sensors_open() call off main thread to reduce boot time. r?dhylands

Bug 1184081 - Move sensors_open() call off main thread to reduce boot time. r?dhylands
Attachment #8634651 - Flags: review?(dhylands)
(Reporter)

Comment 2

2 years ago
(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #0)
> During boot, we call sensors_open() on the main thread, which takes about
> 480 ms. We should move it to the polling thread if it's not required to be
> called on the main thread.
> 
Forgot to mention that this is on flame-kk. In contrast nexus 5 doesn't have this problem and sensors_open() on it is almost instant. But I think it's good to move it off main thread in case the implementation should ever block, given that calling it off main thread is safe.
Comment on attachment 8634651 [details]
MozReview Request: Bug 1184081 - Move sensors_open() call off main thread to reduce boot time. r?dhylands

https://reviewboard.mozilla.org/r/13425/#review12261

::: hal/gonk/GonkSensor.cpp:334
(Diff revision 1)
> +                                                               true));

Why is this posted? If this needs to happen after OpenSensors is called then we need to fix EnableSensorNotifications as well, since it doesn't check to see if OpenSensors has been called, just that the thread was created.

Should we always disapatch SetSensorState to the polling thread? If not, then there could be calls from the main thread and posted calls which are executed in a different order.
Attachment #8634651 - Flags: review?(dhylands)
(Reporter)

Comment 4

2 years ago
https://reviewboard.mozilla.org/r/13425/#review12261

> Why is this posted? If this needs to happen after OpenSensors is called then we need to fix EnableSensorNotifications as well, since it doesn't check to see if OpenSensors has been called, just that the thread was created.
> 
> Should we always disapatch SetSensorState to the polling thread? If not, then there could be calls from the main thread and posted calls which are executed in a different order.

SetSensorState() needs to access sSensorModule, which is initialized in OpenSensors(). The check whether sPollingThread exists also acts as the check whether it's the first time we call EnableSensoeNotifications().

Yes, we should dispatch SetSensorState to the polling thread, but why I am not doing this is that once the polling thread is started, the thread could block in polling the sensor device and the action could be delayed until some sensor responded.

I think what we need to do is creating a control thread for initializing, enabling and disabling the sensors and dispatch to the control requests to that thread.
(Reporter)

Comment 5

2 years ago
Just tested on aries-l and sensors_open() also finishes instantly. So we may lower the priority of this bug and have it a nice-to-have improvement.
Severity: normal → minor
[Tracking Requested - why for this release]:
tracking-b2g: --- → backlog
Priority: -- → P4
(Reporter)

Comment 7

2 years ago
WONTFIX because of bug 1194721.

After the sensor and other code that uses the binary blobs are moved into a separate address space, we won't block on these calls during startup.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
(Reporter)

Updated

2 years ago
Attachment #8634651 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.