Closed Bug 1022746 Opened 6 years ago Closed 6 years ago

The call log isn't using APZC anymore :/

Categories

(Firefox OS Graveyard :: Gaia::Dialer, defect, P1, blocker)

ARM
macOS
defect

Tracking

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: etienne, Assigned: vingtetun)

References

Details

(Keywords: perf, regression, Whiteboard: [c=regression p= s= u=2.0][planned-sprint])

Attachments

(2 files, 1 obsolete file)

This might be a fallout from the visual refresh.

STR:
* Install a reference-workload on the device
* Enable overscrolling in the developer settings
* overscroll in the settings app -> you get a cool effect
* Launch the dialer
* Go in the contacts tab, overscroll -> you get a cool effect
* Go in the call log, overscroll -> you get nothing :/ and the scrolling performance is bad.

Looks like we're doing something that makes fall out of the APZC code path.
APZC + Dialer, this one has your name written all over it Doug :)
Flags: needinfo?(drs+bugzilla)
Oh and I can confirm that everything works fine on earlier versions (tried with 'master@{2014-05-20 18:30:00}').
This is the cause (wat?): https://github.com/mozilla-b2g/gaia/commit/c2e8699c8656ddf21e2d35272c28ecd434a57336
Assignee: nobody → drs+bugzilla
Depends on: 983812
Flags: needinfo?(drs+bugzilla)
Although a work-around would be great, it'd be good to understand why this happens from a gfx/layout perspective too - we shouldn't be getting scrollable layers with no APZC :/ (unless I've wildly misunderstood how things are meant to work)
If I were debugging this, I would first get a compositor-side layer dump to see if the layer is a scroll info layer or a proper scroll layer (see if the thebes layers are children of the container layer or not). If we're getting a scroll info layer (which is the likely scenario), we'd need client-side display list dumps to see why the scroll layer items fail to merge.
I found the issue already and I have a fix for it:
https://github.com/DouglasSherk/gaia/commit/349f723cea09fc2e4228ad47105832bfb1701534

I'm just waiting for the integration tests to run since I think this will cause a regression in the number of reflows.

Though I'm not sure why this is bailing us out of the APZC path, and I haven't looked into that yet.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> If I were debugging this, I would first get a compositor-side layer dump to
> see if the layer is a scroll info layer or a proper scroll layer (see if the
> thebes layers are children of the container layer or not). If we're getting
> a scroll info layer (which is the likely scenario), we'd need client-side
> display list dumps to see why the scroll layer items fail to merge.

The APZC tree log (bug 958596) contains the same information in a less noisy environment. More, if you count the content description string (which regressed recently, but I just posted a fix in bug 1023491).
PR: https://github.com/mozilla-b2g/gaia/pull/20316

This fixes the issue albeit at the cost of some performance, according to the comment. I'm still not sure why this is bailing us out of the APZC code path, but we can investigate that in a followup.
Attachment #8438010 - Flags: review?(21)
Can we get an explanation of how bad the scrolling perf is? A video also would help here.
(In reply to Jason Smith [:jsmith] from comment #9)
> Can we get an explanation of how bad the scrolling perf is? A video also
> would help here.

With reference-workload-medium, I get about 20-30 fps (eyeballing) on my Flame, so it's pretty bad. We also can't overscroll without APZC.
(In reply to Doug Sherk (:drs) from comment #10)
> (In reply to Jason Smith [:jsmith] from comment #9)
> > Can we get an explanation of how bad the scrolling perf is? A video also
> > would help here.
> 
> With reference-workload-medium, I get about 20-30 fps (eyeballing) on my
> Flame, so it's pretty bad. We also can't overscroll without APZC.

Okay, that's a pretty bad perf regression. Noming then.
blocking-b2g: --- → 2.0?
Keywords: qawanted
I'm responsible for the initial code using rAF. The font_size_utils.js script does a reflow, so the idea was to delay this reflow until after the layout gets updated. Not using rAF works too but may in some case causes additional reflows.

I need to understand what's the impact of that on the layout.
Priority: -- → P1
Hardware: x86 → ARM
Whiteboard: [c=regression p= s= u=2.0]
Status: NEW → ASSIGNED
Severity: normal → blocker
blocking-b2g: 2.0? → 2.0+
Comment on attachment 8438010 [details] [diff] [review]
Remove requestAnimationFrame from FontSizeUtils to fix frames falling back to sync panning and zooming.

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

Do we know when this code path is triggered ? It should only be triggered after the app startup and once lazy loaded html elements comes in. And so it should not affects the call log, and especially not once the call log is loaded.

Before reviewing it I would like to check that there is not a bigger bug that is hiding in this code.
Attachment #8438010 - Flags: review?(21)
I double checked and there are not extra calls to functions in font_size_utils after the app is launched.
If, however, I apply this patch, then the call log is butter smooth again and the overscrolling is back.

It's still unclear what's wrong with the rAF.
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #13)
> Do we know when this code path is triggered ? It should only be triggered
> after the app startup and once lazy loaded html elements comes in. And so it
> should not affects the call log, and especially not once the call log is
> loaded.
> 
> Before reviewing it I would like to check that there is not a bigger bug
> that is hiding in this code.

I don't have time to investigate this deeply (especially since gmarty did a shallow investigation of what you suggested), so unfortunately I think this will have to wait. I filed bug 1027869 to follow up on this. Is this ok with you?
Flags: needinfo?(21)
Comment on attachment 8438010 [details] [diff] [review]
Remove requestAnimationFrame from FontSizeUtils to fix frames falling back to sync panning and zooming.

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

Let's go with the followup. I'm not even sure APZ is disabled - as I can overscroll by panning very quicky sometimes, but then it jumps back to the top instantly.
Attachment #8438010 - Flags: review+
https://github.com/mozilla-b2g/gaia/commit/fec2d2661606a8f1f6f9eba93d9ee84f7273db9b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(21)
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S4 (20june)
kats says this might have not fixed it for him.
Flags: needinfo?(drs+bugzilla)
(In reply to Doug Sherk (:drs) from comment #18)
> kats says this might have not fixed it for him.

Yep on a central/master from today with a reference workload the issue is still happening :/
Attached patch bug1022746.patch (obsolete) — Splinter Review
If it works for you as well Etienne, I will suggest to backout the requestAnimationFrame change as well, since it was likely unrelated.
Attachment #8447126 - Flags: review?(etienne)
Basically the issue seems to be a z-indexes nightmare (again...). I don't see why we need them in shared/style/list.css and removed all of them.

I can't see any regressions on the device without them.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8447126 [details] [diff] [review]
bug1022746.patch

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

Looks like this is breaking edit mode, but should be fixable :)
Attachment #8447126 - Flags: review?(etienne)
Arnau, any idea?
Maybe we can pointer-events:none instead of putting elements on top with z-indexes.

Or at least keep the z-index stuff only for the edit-mode, so we get APZC scrolling the rest of the time.
Flags: needinfo?(rnowmrch)
(In reply to Etienne Segonzac (:etienne) from comment #24)
> Arnau, any idea?
> Maybe we can pointer-events:none instead of putting elements on top with
> z-indexes.
> 
> Or at least keep the z-index stuff only for the edit-mode, so we get APZC
> scrolling the rest of the time.

I want APZ all the time :)
Attached patch bug1022746.patchSplinter Review
Seems like the edit mode is specific for each app...
Attachment #8447126 - Attachment is obsolete: true
Attachment #8447172 - Flags: review?(etienne)
Yes, edit mode is a bit messy, and will be redesigned soon to use a toolbar at the bottom.
I installed Attachment #8447126 [details] [diff] and I have seen nothing broken in edit mode, is it anything I'm missing?
Flags: needinfo?(rnowmrch)
Stealing.
Assignee: drs+bugzilla → 21
Whiteboard: [c=regression p= s= u=2.0] → [c=regression p= s= u=2.0][planned-sprint]
Previous target milestone is obsolete, so I'm deferring it to sprint5.
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Comment on attachment 8447172 [details] [diff] [review]
bug1022746.patch

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

So smooth! 
Glad we can do this without z-index hacks.

I'd like this to get it stamped by sms/bb peers though.
Attachment #8447172 - Flags: review?(schung)
Attachment #8447172 - Flags: review?(rnowmrch)
Attachment #8447172 - Flags: review?(etienne)
Attachment #8447172 - Flags: feedback+
Comment on attachment 8447172 [details] [diff] [review]
bug1022746.patch

LGTM.
Vivien, just one thing, could you please also add in lists.css:
[data-type="list"] [data-type="edit"] li > a {
  pointer-events: none;
}

Same thing you are doing in Messages app, in case someone else is using the proposed structure in BB ([data-type="edit"] instead of .edit)
Attachment #8447172 - Flags: review?(rnowmrch) → review+
Comment on attachment 8447172 [details] [diff] [review]
bug1022746.patch

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

Nice catch Etienne! Thanks for avoiding the regression.
Attachment #8447172 - Flags: review?(schung) → review+
https://github.com/mozilla-b2g/gaia/commit/d7f7141d3a2b4238ac29234d08ed1728f0a304b6
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 1057080
You need to log in before you can comment on or make changes to this bug.