952131 - Layout costs a lot for display orientation change.

Status: RESOLVED DUPLICATE of bug 993019

(Core Graveyard :: Widget: Gonk, defect)

Description

[Vincent Lin[:vilin]]

To launch the browser or any app can be switched between portrait and landscape.

As rotating the device, the display reacts so slowly. The profiler shows that layout(DoReflow and FlushPendingNotifications) is one of the bottlenecks which causes slow reaction.

Comment 1

[Inder]

Based on mwu's recommendation in bug 988883#c7 adding this for CS goal.

Comment 2

[Inder]

Tapas -- please provide the profile data requested by mwu in bug 988883.

Comment 3

[Preeti Raghunath(:Preeti)]

Nice to have for CAF now. Need investigation on how to optimize within testing time frame.

Comment 4

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

@mwu:

Are you able to produce ~2sec rotation delay (for video playback use case) on your QRD msm8226 device and v1.4 FFOS ? 

I am mainly concerned about delay between [1] and HWCComposer2D::TryRender() .

[1] https://github.com/mozilla/integration-mozilla-inbound/blob/master/widget/gonk/OrientationObserver.cpp#L253


I followed [2] but I am still seeing some issues to capture gecko profiler data. I was thinking whether you can get the data for video rotation use cases on QRD msm8x26 device. Please confirm me.

[2] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler#Capture_the_Profile

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Jerry made a recent fix to the orientation related performance issue, but I guess this is an issue with the latest 1.4?

Comment 6

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Milan Sreckovic [:milan] from comment #5)
> Jerry made a recent fix to the orientation related performance issue, but I
> guess this is an issue with the latest 1.4?

patch from https://bugzilla.mozilla.org/show_bug.cgi?id=993019 has reduced the delay which I observed in #comment 4.

But there is one more minor fix which can save some random delay .

We are calling Reset() [1] whenever sensor event timestamp exceeds 32bit. Current we are using "long" data type to store or calculate timestamp in ProcessOrientation.cpp and ProcessOrientation.h . We need to change all such "long" datatype to PRInt64 which is 8byte. This will prevent some random delay.

once this is done then we can close this issue :) 

[1] http://dxr.mozilla.org/mozilla-central/source/widget/gonk/ProcessOrientation.cpp#193

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Tapas Kumar Kundu from comment #6)
> patch from https://bugzilla.mozilla.org/show_bug.cgi?id=993019 has reduced
> the delay which I observed in #comment 4.

If bug 993019 can improve something, that's good.
Bug 1003870 is also related to the rotation performance.

> But there is one more minor fix which can save some random delay .
> 
> We are calling Reset() [1] whenever sensor event timestamp exceeds 32bit.
> Current we are using "long" data type to store or calculate timestamp in
> ProcessOrientation.cpp and ProcessOrientation.h . We need to change all such
> "long" datatype to PRInt64 which is 8byte. This will prevent some random
> delay.
> 
> once this is done then we can close this issue :) 

ex:
The number of seconds since 1-1-1970: 1399344736.
We get time stamp data at [1]. The value is 1399344736*10^6 [2]. We got overflow here.

[1]
http://dxr.mozilla.org/mozilla-central/source/widget/gonk/ProcessOrientation.cpp#184
[2]
http://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prtime.h#48(In reply to Tapas Kumar Kundu from comment #6)

Tapas, you are the original author for this module, do you want to take this task?

Comment 8

[bhavana bajaj [:bajaj]]

(In reply to Preeti Raghunath(:Preeti) from comment #3)
> Nice to have for CAF now. Need investigation on how to optimize within
> testing time frame.

Can we remove this from blocking 1.4 as this is a nice-to-have ?

Comment 9

[Preeti Raghunath(:Preeti)]

Fix coming from partner. Confirming Moz' involvement clearly here. Hold off on changing flags.

Comment 10

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: Bug 952131 - Layout costs a lot for display orientation change r=mwu

pls review

Comment 11

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: Bug 952131 - Layout costs a lot for display orientation change r=mwu

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8418467 [details] [diff] [review]
Bug 952131 - Layout costs a lot for display orientation change r=mwu

Review of attachment 8418467 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/ProcessOrientation.cpp
@@ +414,5 @@
>  
>  int
>  ProcessOrientation::Reset()
>  {
> +  mLastFilteredTimestampNanos = LLONG_MIN;

Should we use std::numeric_limits instead of the specific LLONG type?
It's more portable.

std::numeric_limits<PRTime>::min()
std::numeric_limits<PRTime>::max()

Comment 13

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Comment on attachment 8418467 [details] [diff] [review]
Bug 952131 - Layout costs a lot for display orientation change r=mwu

changed reviewer :)

Comment 14

[Michael Vines [:m1] [:evilmachines]]

:tk, I think :mwu is on PTO this week.  Looks like you have :jerry's attention though :)

Comment 15

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Comment on attachment 8418467 [details] [diff] [review]
Bug 952131 - Layout costs a lot for display orientation change r=mwu

I will update a new patch soon

Comment 16

[Andreas Gal :gal]

I recommend not tagging me for review. I usually drown in emails. Sorry about the slow response.

Comment 17

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Comment on attachment 8418467 [details] [diff] [review]
Bug 952131 - Layout costs a lot for display orientation change r=mwu

># HG changeset patch
># User Tapas Kundu <[e-mail]>
># Date 1399425317 25200
># Node ID 86631265a6c8058cc36d46b1bf48280fb7b833e0
># Parent  8f4c8f01f554c338bbcb6f3df054c7be90790102
>Bug 952131 - Layout costs a lot for display orientation change r=mwu
>
>diff --git a/widget/gonk/ProcessOrientation.cpp b/widget/gonk/ProcessOrientation.cpp
>--- a/widget/gonk/ProcessOrientation.cpp
>+++ b/widget/gonk/ProcessOrientation.cpp
>@@ -10,17 +10,16 @@
>  *      http://www.apache.org/licenses/LICENSE-2.0
>  *
>  * Unless required by applicable law or agreed to in writing, software
>  * distributed under the License is distributed on an "AS IS" BASIS,
>  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>  * See the License for the specific language governing permissions and
>  * limitations under the License.
>  */
>-
> #include "base/basictypes.h"
> #include "mozilla/Hal.h"
> #include "nsIScreen.h"
> #include "nsIScreenManager.h"
> #include "OrientationObserver.h"
> #include "ProcessOrientation.h"
> #include "mozilla/HalSensor.h"
> #include "math.h"
>@@ -176,18 +175,18 @@ ProcessOrientation::OnSensorChanged(cons
> 
>   LOGD
>     ("ProcessOrientation: Raw acceleration vector: x = %f, y = %f, z = %f,"
>      "magnitude = %f\n", x, y, z, sqrt(x * x + y * y + z * z));
>   // Apply a low-pass filter to the acceleration up vector in cartesian space.
>   // Reset the orientation listener state if the samples are too far apart in
>   // time or when we see values of (0, 0, 0) which indicates that we polled the
>   // accelerometer too soon after turning it on and we don't have any data yet.
>-  const long now = event.timestamp();
>-  const long then = mLastFilteredTimestampNanos;
>+  const PRInt64 now = (PRInt64) event.timestamp();
>+  const PRInt64 then = mLastFilteredTimestampNanos;
>   const float timeDeltaMS = (now - then) * 0.000001f;
>   bool skipSample = false;
>   if (now < then
>       || now > then + MAX_FILTER_DELTA_TIME_NANOS
>       || (x == 0 && y == 0 && z == 0)) {
>     LOGD
>       ("ProcessOrientation: Resetting orientation listener.");
>     Reset();
>@@ -383,17 +382,17 @@ ProcessOrientation::IsOrientationAngleAc
>         return false;
>       }
>     }
>   }
>   return true;
> }
> 
> bool
>-ProcessOrientation::IsPredictedRotationAcceptable(long now)
>+ProcessOrientation::IsPredictedRotationAcceptable(PRInt64 now)
> {
>   // The predicted rotation must have settled long enough.
>   if (now < mPredictedRotationTimestampNanos + PROPOSAL_SETTLE_TIME_NANOS) {
>     return false;
>   }
>   // The last flat state (time since picked up) must have been sufficiently long
>   // ago.
>   if (now < mFlatTimestampNanos + PROPOSAL_MIN_TIME_SINCE_FLAT_ENDED_NANOS) {
>@@ -411,35 +410,35 @@ ProcessOrientation::IsPredictedRotationA
>   }
>   // Looks good!
>   return true;
> }
> 
> int
> ProcessOrientation::Reset()
> {
>-  mLastFilteredTimestampNanos = std::numeric_limits<PRInt64>::min();
>+  mLastFilteredTimestampNanos = std::numeric_limits<PRInt64>::min();
>   mProposedRotation = -1;
>-  mFlatTimestampNanos = std::numeric_limits<PRInt64>::min();
>-  mSwingTimestampNanos = std::numeric_limits<PRInt64>::min();
>-  mAccelerationTimestampNanos = std::numeric_limits<PRInt64>::min();
>+  mFlatTimestampNanos = std::numeric_limits<PRInt64>::min();
>+  mSwingTimestampNanos = std::numeric_limits<PRInt64>::min();
>+  mAccelerationTimestampNanos = std::numeric_limits<PRInt64>::min();
>   ClearPredictedRotation();
>   ClearTiltHistory();
>   return -1;
> }
> 
> void
> ProcessOrientation::ClearPredictedRotation()
> {
>   mPredictedRotation = -1;
>-  mPredictedRotationTimestampNanos = std::numeric_limits<PRInt64>::min();
>+  mPredictedRotationTimestampNanos = std::numeric_limits<PRInt64>::min();
> }
> 
> void
>-ProcessOrientation::UpdatePredictedRotation(long now, int rotation)
>+ProcessOrientation::UpdatePredictedRotation(PRInt64 now, int rotation)
> {
>   if (mPredictedRotation != rotation) {
>     mPredictedRotation = rotation;
>     mPredictedRotationTimestampNanos = now;
>   }
> }
> 
> bool
>@@ -447,46 +446,46 @@ ProcessOrientation::IsAccelerating(float
> {
>   return magnitude < MIN_ACCELERATION_MAGNITUDE
>     || magnitude > MAX_ACCELERATION_MAGNITUDE;
> }
> 
> void
> ProcessOrientation::ClearTiltHistory()
> {
>-  mTiltHistory.history[0].timestampNanos = std::numeric_limits<PRInt64>::min();
>+  mTiltHistory.history[0].timestampNanos = std::numeric_limits<PRInt64>::min();
>   mTiltHistory.index = 1;
> }
> 
> void
>-ProcessOrientation::AddTiltHistoryEntry(long now, float tilt)
>+ProcessOrientation::AddTiltHistoryEntry(PRInt64 now, float tilt)
> {
>   mTiltHistory.history[mTiltHistory.index].tiltAngle = tilt;
>   mTiltHistory.history[mTiltHistory.index].timestampNanos = now;
>   mTiltHistory.index = (mTiltHistory.index + 1) % TILT_HISTORY_SIZE;
>-  mTiltHistory.history[mTiltHistory.index].timestampNanos = std::numeric_limits<PRInt64>::min();
>+  mTiltHistory.history[mTiltHistory.index].timestampNanos = std::numeric_limits<PRInt64>::min();
> }
> 
> bool
>-ProcessOrientation::IsFlat(long now)
>+ProcessOrientation::IsFlat(PRInt64 now)
> {
>   for (int i = mTiltHistory.index; (i = NextTiltHistoryIndex(i)) >= 0;) {
>     if (mTiltHistory.history[i].tiltAngle < FLAT_ANGLE) {
>       break;
>     }
>     if (mTiltHistory.history[i].timestampNanos + FLAT_TIME_NANOS <= now) {
>       // Tilt has remained greater than FLAT_TILT_ANGLE for FLAT_TIME_NANOS.
>       return true;
>     }
>   }
>   return false;
> }
> 
> bool
>-ProcessOrientation::IsSwinging(long now, float tilt)
>+ProcessOrientation::IsSwinging(PRInt64 now, float tilt)
> {
>   for (int i = mTiltHistory.index; (i = NextTiltHistoryIndex(i)) >= 0;) {
>     if (mTiltHistory.history[i].timestampNanos + SWING_TIME_NANOS < now) {
>       break;
>     }
>     if (mTiltHistory.history[i].tiltAngle + SWING_AWAY_ANGLE_DELTA <= tilt) {
>       // Tilted away by SWING_AWAY_ANGLE_DELTA within SWING_TIME_NANOS.
>       return true;
>@@ -494,18 +493,18 @@ ProcessOrientation::IsSwinging(long now,
>   }
>   return false;
> }
> 
> int
> ProcessOrientation::NextTiltHistoryIndex(int index)
> {
>   index = (index == 0 ? TILT_HISTORY_SIZE : index) - 1;
>-  return mTiltHistory.history[index].timestampNanos != std::numeric_limits<PRInt64>::min() ? index : -1;
>+  return mTiltHistory.history[index].timestampNanos != std::numeric_limits<PRInt64>::min() ? index : -1;
> }
> 
> float
>-ProcessOrientation::RemainingMS(long now, long until)
>+ProcessOrientation::RemainingMS(PRInt64 now, PRInt64 until)
> {
>   return now >= until ? 0 : (until - now) * 0.000001f;
> }
> 
> } // namespace mozilla
>diff --git a/widget/gonk/ProcessOrientation.h b/widget/gonk/ProcessOrientation.h
>--- a/widget/gonk/ProcessOrientation.h
>+++ b/widget/gonk/ProcessOrientation.h
>@@ -44,67 +44,67 @@ private:
>   // Returns true if the orientation angle is acceptable for a given predicted
>   // rotation. This function takes into account the gap between adjacent
>   // orientations for hysteresis.
>   bool IsOrientationAngleAcceptable(int rotation, int orientationAngle,
>                                     int currentRotation);
> 
>   // Returns true if the predicted rotation is ready to be advertised as a
>   // proposed rotation.
>-  bool IsPredictedRotationAcceptable(long now);
>+  bool IsPredictedRotationAcceptable(PRInt64 now);
> 
>   void ClearPredictedRotation();
>-  void UpdatePredictedRotation(long now, int rotation);
>+  void UpdatePredictedRotation(PRInt64 now, int rotation);
>   bool IsAccelerating(float magnitude);
>   void ClearTiltHistory();
>-  void AddTiltHistoryEntry(long now, float tilt);
>-  bool IsFlat(long now);
>-  bool IsSwinging(long now, float tilt);
>+  void AddTiltHistoryEntry(PRInt64 now, float tilt);
>+  bool IsFlat(PRInt64 now);
>+  bool IsSwinging(PRInt64 now, float tilt);
>   int NextTiltHistoryIndex(int index);
>-  float RemainingMS(long now, long until);
>+  float RemainingMS(PRInt64 now, PRInt64 until);
> 
>   // The tilt angle range in degrees for each orientation. Beyond these tilt
>   // angles, we don't even consider transitioning into the specified orientation.
>   // We place more stringent requirements on unnatural orientations than natural
>   // ones to make it less likely to accidentally transition into those states.
>   // The first value of each pair is negative so it applies a limit when the
>   // device is facing down (overhead reading in bed). The second value of each
>   // pair is positive so it applies a limit when the device is facing up
>   // (resting on a table). The ideal tilt angle is 0 (when the device is vertical)
>   // so the limits establish how close to vertical the device must be in order
>   // to change orientation.
>   static const int tiltTolerance[][4];
> 
>   // Timestamp and value of the last accelerometer sample.
>-  long mLastFilteredTimestampNanos;
>+  PRInt64 mLastFilteredTimestampNanos;
>   float mLastFilteredX, mLastFilteredY, mLastFilteredZ;
> 
>   // The last proposed rotation, -1 if unknown.
>   int mProposedRotation;
> 
>   // Value of the current predicted rotation, -1 if unknown.
>   int mPredictedRotation;
> 
>   // Timestamp of when the predicted rotation most recently changed.
>-  long mPredictedRotationTimestampNanos;
>+  PRInt64 mPredictedRotationTimestampNanos;
> 
>   // Timestamp when the device last appeared to be flat for sure (the flat delay
>   // elapsed).
>-  long mFlatTimestampNanos;
>+  PRInt64 mFlatTimestampNanos;
> 
>   // Timestamp when the device last appeared to be swinging.
>-  long mSwingTimestampNanos;
>+  PRInt64 mSwingTimestampNanos;
> 
>   // Timestamp when the device last appeared to be undergoing external
>   // acceleration.
>-  long mAccelerationTimestampNanos;
>+  PRInt64 mAccelerationTimestampNanos;
> 
>   struct {
>     struct {
>       float tiltAngle;
>-      long timestampNanos;
>+      PRInt64 timestampNanos;
>     } history[TILT_HISTORY_SIZE];
>     int index;
>   } mTiltHistory;
> };
> 
> } // namespace mozilla
> #endif

Comment 18

[Michael Wu [:mwu]]

Just got back from PTO.

s/PRInt64/int64_t/ - we prefer to use standard types rather than NSPR types now.

BTW this patch doesn't fix issues with layout being slow, so I recommend spinning out a new bug specifically for the issue of the wrong type being used.

Comment 19

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Creating a new #bug 1007349 for #comment 18