939392 - [Homescreen] Invisible border causes layers to be much bigger

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

Because of this invisible border we extend the size of the layer to the bottom of the page. Without this border I can get the size of the layer on hidpi device to much smaller.

crdlc can you elaborate why we have this border? It seems to be slowing things down.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Screenshot - Reduce layer size

Notice how the layer is now just the upper 20% portion of the screen.

Comment 2

[Cristian Rodriguez (:crdlc)]

Comment on attachment 833315 [details] [diff] [review]
patch

Very simple, it was added 3 or more months ago because when we changed adjacent pages from display: none to display: block, they were invisible (not repainted)

Comment 3

[Vincent Lin[:vilin]]

(In reply to Cristian Rodriguez (:crdlc) from comment #2)
> Comment on attachment 833315 [details] [diff] [review]
> patch
> 
> Very simple, it was added 3 or more months ago because when we changed
> adjacent pages from display: none to display: block, they were invisible
> (not repainted)

Are you talking about Bug 842218 ?

We've experimented on HD display device.
Removing "border-bottom" did significantly reduce painting time and stutter issue is improved a lot.

Even if not removing "border-bottom", what we found was the previous/next pages were actually not pre-rendered in memory(2nd approach of https://bugzilla.mozilla.org/show_bug.cgi?id=842218#c16). Repainting still happened when swiping homescreen pages. Does it still meet your expectations on the fix for Bug 842218.

We're also struggling on this kind of issue, especially for HD display device. Bug 920921 records our course during the past 1~2 months.

Comment 4

[Cristian Rodriguez (:crdlc)]

(In reply to Vincent Lin[:vincentlin] from comment #3)
> (In reply to Cristian Rodriguez (:crdlc) from comment #2)
> > Comment on attachment 833315 [details] [diff] [review]
> > patch
> > 
> > Very simple, it was added 3 or more months ago because when we changed
> > adjacent pages from display: none to display: block, they were invisible
> > (not repainted)
> 
> Are you talking about Bug 842218 ?

Yes, so 8-9 months ago

> 
> We've experimented on HD display device.
> Removing "border-bottom" did significantly reduce painting time and stutter
> issue is improved a lot.
> 

Good news

> Even if not removing "border-bottom", what we found was the previous/next
> pages were actually not pre-rendered in memory(2nd approach of
> https://bugzilla.mozilla.org/show_bug.cgi?id=842218#c16). Repainting still
> happened when swiping homescreen pages. Does it still meet your expectations
> on the fix for Bug 842218.

:( At that moment it happened, adjacent pages go to display block and when they are translated following the finger, sometimes were invisible. I added that border and the issue was fixed. AFAIR if you add rgba(0,0,0,0.0.1) as background color to pages, there were visible as well like with border happens. I am not pretty sure but I this so

> 
> We're also struggling on this kind of issue, especially for HD display
> device. Bug 920921 records our course during the past 1~2 months.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: Pull request 13853

Comment 6

[Benoit Girard (:BenWa)]

Not sure if I need to wait until Travis completes before checkin-needed. Let me know.

Comment 7

[Reuben Morais [:reuben]]

merged: https://github.com/mozilla-b2g/gaia/commit/019275e2e193ce7e9b5ac5f362af93ac0773722a

Comment 8

[Benoit Girard (:BenWa)]

Requesting koi+ since this is a big performance win.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Benoit Girard (:BenWa) from comment #8)
> Requesting koi+ since this is a big performance win.

What perf metrics would this help improve? FPS?

Comment 10

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

Benoit, what is the performance win, and do we have numbers?

Comment 11

[Benoit Girard (:BenWa)]

Vincent you collect data for this I saw on gdocs. Can you share this publicly here?

(In reply to Milan Sreckovic [:milan] from comment #10)
> Benoit, what is the performance win, and do we have numbers?

Vincent has numbers. We saw that the allocation time (latency for the first 2 frames) was significantly reduced.

In any case it will save a half screen size buffer x 2 buffer x 2 buffer per layer * 4 bytes per pixel = ~2MB of gralloc.

Comment 12

[Vincent Lin[:vilin]]

Profiling for the cost of "nsLayoutUtils::PaintFrame()" with 100 times sampling statistics.

w/t border-bottom
avg:202.980 ms, max:931.126 ms, min:4.438 ms, std:209.756

w/o border-bottom
avg:75.376 ms, max:474.395 ms, min:4.555 ms, std:99.589
(Note: the min is the time for the painting of Clock(Top-Right corner) during my profiling process.)

It's a significant improvement for stutter issue(The lag at the beginning of swiping homescreen).

But for transforming between two pages, it's irrelevant to this fix because there's no painting here.
I just create Bug 941984 for it.

Comment 13

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

We know we're running into OOM situations, so 2MB is not something we should ignore. I'd say it's worth being koi+.

Comment 14

[bhavana bajaj [:bajaj]]

Given the perf win and a simple fix, taking this,

Comment 15

[Jason Smith [:jsmith]]

Reuben - Shouldn't this be closed as resolved fixed if this already landed in the above comment?

Comment 16

[Benoit Girard (:BenWa)]

Attachment: Uplift to v1.2

Comment 17

[Benoit Girard (:BenWa)]

Do we need additional approval step before merging?

Comment 18

[Reuben Morais [:reuben]]

(In reply to Jason Smith [:jsmith] from comment #15)
> Reuben - Shouldn't this be closed as resolved fixed if this already landed
> in the above comment?

Yes, every time I land a Gaia commit I forget it doesn't work like inbound :P

Comment 19

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 019275e2e193ce7e9b5ac5f362af93ac0773722a to:
v1.3 already had this commit
v1.2: cd7b3e6889c427f3068063b97498b0b6c6dcc7f5