1127170 - content layer transaction spends more than 150ms when switch app back to homescreen

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Performance, defect, P2)

Description

[Peter Chang[:pchang]]

With FPS enable, you can see a visual warning rect displayed when switch app back to homescreen. It indicates a long transaction delay (>150ms) happened.

Comment 1

[Peter Chang[:pchang]]

After checking app switch to homescreen in systrace, there are two long transaction is sent by homescreen app.

The first one is triggered because JS wants to active homescreen which calls to TabChild::MakeVisiable.
The second one is triggered by RefreshDriver::Tick. Both spend around 200 ms for painting.

I'm checking there are any duplicate displaylist::Draw tasks or not.

Comment 2

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #1)
> After checking app switch to homescreen in systrace, there are two long
> transaction is sent by homescreen app.
> 
> The first one is triggered because JS wants to active homescreen which calls
> to TabChild::MakeVisiable.
> The second one is triggered by RefreshDriver::Tick. Both spend around 200 ms
> for painting.
> 
> I'm checking there are any duplicate displaylist::Draw tasks or not.

The following are the displaylist dump for these two long transaction of home screen.
After dumping the displaylist content, I found the icons' text were be painted twice. 

http://pastebin.mozilla.org/8633080

Comment 3

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #2)
> (In reply to peter chang[:pchang][:peter] from comment #1)
> > After checking app switch to homescreen in systrace, there are two long
> > transaction is sent by homescreen app.
> > 
> > The first one is triggered because JS wants to active homescreen which calls
> > to TabChild::MakeVisiable.
> > The second one is triggered by RefreshDriver::Tick. Both spend around 200 ms
> > for painting.
> > 
> > I'm checking there are any duplicate displaylist::Draw tasks or not.
> 
> The following are the displaylist dump for these two long transaction of
> home screen.
> After dumping the displaylist content, I found the icons' text were be
> painted twice. 
> 
> http://pastebin.mozilla.org/8633080

Above sample is done with tiling.

If I disable the tiling, only the text in the second page of homescreen will be re-painted.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Can you get a profile of this happening?

Comment 5

[Peter Chang[:pchang]]

I just found the icons in homescreen triggered lots of InvalidateImagesCallback and each displayitem set as invalid[1] in the second transaction. And those text items will be re-painted again.

[1]https://dxr.mozilla.org/mozilla-central/source/layout/style/ImageLoader.cpp#353

Comment 6

[Peter Chang[:pchang]]

Jeff, the following is the profile link.

http://people.mozilla.org/~bgirard/cleopatra/#report=cfe5ee2fcab1403cc4fef0a569c3043cce880f83

Comment 7

[Peter Chang[:pchang]]

Paste the profiler link after removing the Display List label.

For the shadow text, I saw it only cost about 2~3ms but there are many different text items need to be painted.

http://people.mozilla.org/~bgirard/cleopatra/#report=c8fe587e85c41cb5fea887f60ef0cae8ee1b9f14

Comment 8

[Peter Chang[:pchang]]

Now I'm using gecko profiler to check the display list since gecko profiler provides the display list visualization.

Comment 9

[Peter Chang[:pchang]]

From the profiler in comment 6, the first long transaction loading of homescreen didn't display the 'DisplayList' Marker because this pending update is triggered from [1], not from refresh driver tick function[2].


[1]https://dxr.mozilla.org/mozilla-central/source/view/nsViewManager.cpp#678
[2]https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp#1708

Comment 10

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #9)
> From the profiler in comment 6, the first long transaction loading of
> homescreen didn't display the 'DisplayList' Marker because this pending
> update is triggered from [1], not from refresh driver tick function[2].
> 
> 
> [1]https://dxr.mozilla.org/mozilla-central/source/view/nsViewManager.cpp#678
> [2]https://dxr.mozilla.org/mozilla-central/source/layout/base/
> nsRefreshDriver.cpp#1708

Create bug 1137109 to address this problem.

Comment 11

[Peter Chang[:pchang]]

Recently I found the gaia in master and v2.2 had different behavior when set homescreen as visible.

In master, the frame tree is changed between the first and second transactions which introduces more painting transactions. This behavior is related to bug 1117444 which tries to change the overflow style of body between hidden and visible.

Comment 12

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #5)
> I just found the icons in homescreen triggered lots of
> InvalidateImagesCallback and each displayitem set as invalid[1] in the
> second transaction. And those text items will be re-painted again.
> 
> [1]https://dxr.mozilla.org/mozilla-central/source/layout/style/ImageLoader.
> cpp#353
Just found out the reason why the text got repainted even its visible region was not overlapped with icon image.

For current home screen app, it creates the following div to include both icon and text.
I tried to modify the margin-top of text as 88px to make sure no overlap between icon and text.
But the text still got re-painted after InvalidateImagesCallback was received.

After checking more, I found the InvalidateImagesCallback was invoked through [1] which tried to enumerate the display items based on the same frame. According to below display items, homescreen received two display items invalidation, nsDisplayTransform and Background types.

That's why the text got re-painted after icon was painted.

As mentioned in comment 5, homescreen
<div class="icon" style="background-image:xxxxx; background-size:88px; transform:xxxxx">
  <p margin-top=86px>
    <span class=title> CrystalSkull </span>
  </p>
</div>

nsDisplayTransform p=0xb33f1810 f=0xb27e48c8(Block(div)(23) class:icon) z=3 bounds(6960,52560,5280,7940) layerBounds(6960,52560,5280,7940) visible(11520,52560,720,7940) layer=0xb188ec80
  Background p=0xb3385668 f=0xb27e48c8(Block(div)(23) class:icon) z=3 bounds(560,0,5280,5280) layerBounds(560,0,5280,5280) visible(5120,0,720,5280) layer=0xb266aa00
  Text p=0xb3385760 f=0xb27e5288(Text(0)"CubeVid") bounds(860,5660,4920,2800) layerBounds(860,5660,4920,2800) visible(5120,6300,660,1640) layer=0xb266aa00


[1]https://dxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#1521

Comment 13

[Peter Chang[:pchang]]

Attachment: Invalidate frames only for display items with background type

Comment 14

[Peter Chang[:pchang]]

Attachment: gaia patch to increate the margin-top to 88px

Comment 15

[Peter Chang[:pchang]]

This bug requires the attachment 8580496 [details] [diff] [review] and 8580497 to work.

Comment 16

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #14)
> Created attachment 8580497 [details] [diff] [review]
> gaia patch to increate the margin-top to 88px

George, as we discussed before, the gaia only needs to change the margin-top of text in homescreen and then we can save some painting cost for text.

Comment 17

[Peter Chang[:pchang]]

Could you have someone to help on the patch?

Comment 18

[Peter Chang[:pchang]]

Update two profiler link

[With my patch]
http://people.mozilla.org/~bgirard/cleopatra/#report=1ecfc90d369819603a4ffe150bdf5d8ad02bb1a7&filter=[{"type"%3A"RangeSampleFilter","start"%3A2391,"end"%3A2571}]

[Without my patch]
http://people.mozilla.org/~bgirard/cleopatra/#report=c8712aece9f2d27a2ce14952b3831bf50b6f6e1b&filter=[{"type"%3A"RangeSampleFilter","start"%3A94653,"end"%3A94871}]

The rasterization reduces from 91ms to 21 ms.

BTW, the text re-paint problem also happened before app launch, therefore, it might help some app launch time.

Comment 19

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8580496 [details] [diff] [review]
Invalidate frames only for display items with background type

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

::: layout/style/ImageLoader.cpp
@@ +347,3 @@
>  
> +  // Bypass frame invalidation when match a display item with non-background type
> +  if (type != nsDisplayItem::TYPE_BACKGROUND) {

Can you explain this change more please.

The problem is that display items other than TYPE_BACKGROUND might render the image that was just invalidated (TYPE_IMAGE, TYPE_CANVAS_BACKGROUND, TYPE_XUL_IMAGE, TYPE_BORDER and quite a few more).

I think you need to see what display item types you're encountering that are triggering this invalidation, and add the TYPE_RENDERS_NO_IMAGES flag (in nsDisplayItemTypesList.h) if they can't render an image as part of the ::Paint() impl.

If we're getting invalidations from items that might paint an image, but just don't happen to paint *this* image then we'll need to consider something more complex.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Looking at comment #12, it seems that TYPE_TRANSFORM is what's causing your invalidation. Adding TYPE_RENDERS_NO_IMAGES to that entry seems like the right thing to do here.

Comment 21

[Peter Chang[:pchang]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> Comment on attachment 8580496 [details] [diff] [review]
> Invalidate frames only for display items with background type
> 
> Review of attachment 8580496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/ImageLoader.cpp
> @@ +347,3 @@
> >  
> > +  // Bypass frame invalidation when match a display item with non-background type
> > +  if (type != nsDisplayItem::TYPE_BACKGROUND) {
> 
> Can you explain this change more please.
> 
> The problem is that display items other than TYPE_BACKGROUND might render
> the image that was just invalidated (TYPE_IMAGE, TYPE_CANVAS_BACKGROUND,
> TYPE_XUL_IMAGE, TYPE_BORDER and quite a few more).
> 
Yes, you are right. I should consider other types which needs repaint for images.
(In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> Looking at comment #12, it seems that TYPE_TRANSFORM is what's causing your
> invalidation. Adding TYPE_RENDERS_NO_IMAGES to that entry seems like the
> right thing to do here.
Yes, adding TYPE_RENDERS_NO_IMAGES to TYPE_TRANSFORM is better. But it might introduces unnecessary invalidation for other display item types because we don't add this flag to all display items which are not related to image.

For me, I think it might help to add some debug log inside the InvalidateImagesCallback when InvalidationDebugging is enabled.

Comment 22

[Matt Woodrow (:mattwoodrow)]

(In reply to peter chang[:pchang][:peter] from comment #21)

> Yes, adding TYPE_RENDERS_NO_IMAGES to TYPE_TRANSFORM is better. But it might
> introduces unnecessary invalidation for other display item types because we
> don't add this flag to all display items which are not related to image.
> 
> For me, I think it might help to add some debug log inside the
> InvalidateImagesCallback when InvalidationDebugging is enabled.

Yeah, going through and figuring out all the items that could use TYPE_RENDER_NO_IMAGES would be great, but it's a lot of work and we have no real evidence that it would help much.

Invalidation logging is an awesome idea though, will save time in the future.

Comment 23

[Peter Chang[:pchang]]

Attachment: Add TYPE_RENDERS_NO_IMAGES for display item with transform type

1. Add TYPE_RENDERS_NO_IMAGES into display item with transform type
2. Add invalidation debug log to indicate which display item is going to invalidate.

For item 2, I already saw several different type of display items got invalidation.
 
I/Gecko   (31450): Invalidating display item(type=34) based on frame 0xb3c46e90
I/Gecko   (31450): Invalidating display item(type=34) based on frame 0xb3c46e90
I/Gecko   (31450): Invalidating display item(type=2) based on frame 0xb3c46e90
I/Gecko   (31450): Invalidating display item(type=2) based on frame 0xb2e687e8
I/Gecko   (31450): Invalidating display item(type=2) based on frame 0xb2e687e8
I/Gecko   (30945): Invalidating display item(type=2) based on frame 0xb3cc58a8
I/Gecko   (31030): Invalidating display item(type=10) based on frame 0xb2cace60
I/Gecko   (31030): Invalidating display item(type=10) based on frame 0xb2cace60
I/Gecko   (31030): Invalidating display item(type=10) based on frame 0xb35bcc28
I/Gecko   (31030): Invalidating display item(type=10) based on frame 0xb35bcc28
I/Gecko   (31030): Invalidating display item(type=10) based on frame 0xb35bd6e8
I/Gecko   (31030): Invalidating display item(type=10) based on frame 0xb35bd6e8
I/Gecko   (30767): Invalidating display item(type=10) based on frame 0xaf326e58
I/Gecko   (30767): Invalidating display item(type=10) based on frame 0xaf326e58
I/Gecko   (31198): Invalidating display item(type=2) based on frame 0xb3cbe550
I/Gecko   (31198): Invalidating display item(type=2) based on frame 0xb3cbe550
I/Gecko   (31198): Invalidating display item(type=10) based on frame 0xb3cbd090
I/Gecko   (31198): Invalidating display item(type=12) based on frame 0xb3cbd090
I/Gecko   (31198): Invalidating display item(type=10) based on frame 0xb3cbd090
I/Gecko   (31198): Invalidating display item(type=12) based on frame 0xb3cbd090

Comment 24

[Peter Chang[:pchang]]

Comment on attachment 8580543 [details] [diff] [review]
Add TYPE_RENDERS_NO_IMAGES for display item with transform type

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

::: layout/style/ImageLoader.cpp
@@ +343,5 @@
>  void InvalidateImagesCallback(nsIFrame* aFrame, 
>                                FrameLayerBuilder::DisplayItemData* aItem)
>  {
>    nsDisplayItem::Type type = nsDisplayItem::GetDisplayItemTypeFromKey(aItem->GetDisplayItemKey());
> +

I will remove this redundant new line in next patch

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8580543 [details] [diff] [review]
Add TYPE_RENDERS_NO_IMAGES for display item with transform type

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

::: layout/style/ImageLoader.cpp
@@ +351,5 @@
>      return;
>    }
>  
> +  if (nsLayoutUtils::InvalidationDebuggingIsEnabled()) {
> +    printf_stderr("Invalidating display item(type=%d) based on frame %p\n",

How about adding "because it might contain an invalidated image" to the end?

Comment 26

[Peter Chang[:pchang]]

Attachment: Add TYPE_RENDERS_NO_IMAGES for display item with transform type

Update debug log based on reviewer suggestion

Comment 27

[Peter Chang[:pchang]]

Attachment: gaia patch to increase the margin-top to 88px

Sam, based on comment 12, I found the texts and the icons of verticalhome were overlapped. And it introduced some unnecessary painting cost. 

  If I make the margin-top of text to 88px, then there are no overlap between texts and icons. Then we can save the invalidation of text when image gets invalidation requests. Let me know if you have any concern about this change.

Comment 28

[Sam Foster [:sfoster] (he/him)]

Attachment: Before/after overlaid screenshots

I think this is a visual/UX call. Bug 1023863 was an exercise in visual consistency and tightening up the grid to match the specs. This patch reverts some of that - which might be a good trade-off. To my eyes the result looks fine. Eric?

Comment 29

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8581467 [details] [diff] [review]
gaia patch to increase the margin-top to 88px

LGTM, but lets hear from Eric.

Comment 30

[Eric Pang [:epang] UX]

Comment on attachment 8581722 [details]
Before/after overlaid screenshots

Redirecting to Peter since he knows the home screen grid and reviewed bug 1023863.  Peter can you take a look?  Thanks!

Comment 31

[Peter Chang[:pchang]]

(In reply to Eric Pang [:epang] from comment #30)
> Comment on attachment 8581722 [details]
> Before/after overlaid screenshots
> 
> Redirecting to Peter since he knows the home screen grid and reviewed bug
> 1023863.  Peter can you take a look?  Thanks!

Loop Taipei UX,
Mike, do you know who can also help me to comment above changes?

Comment 32

[GoodMike]

Hi Harly,
Please sync with Peter and see how we an help. Thank you.

Comment 33

[Peter La]

Comment on attachment 8581722 [details]
Before/after overlaid screenshots

Hi,

This looks fine to me with respect to the icon spacing and gap at the top.  The text labels became lower post-patch, but it looks ok in this screenshot.

Thanks!

Comment 34

[Sam Foster [:sfoster] (he/him)]

Peter, can you create a github pull request for attachment #8581467 [details] [diff] [review] and ask for review from kgrandon?

Comment 35

[Harly Hsu[:harly]]

Thank you both Peter for providing the background and for doing the quick review :)

Comment 36

[Peter Chang[:pchang]]

Attachment: [gaia]howareyou322:bug1127170> mozilla-b2g:master

Create a pull request for gaia changes.

Comment 37

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8586516 [details] [review]
[gaia]howareyou322:bug1127170> mozilla-b2g:master

Great thanks. We'll need a review stamp from a peer to land. Suggesting kgrandon

Comment 38

[Kevin Grandon :kgrandon]

Comment on attachment 8586516 [details] [review]
[gaia]howareyou322:bug1127170> mozilla-b2g:master

I am not happy with the text movement unless there is a very good reason to do so. We spent hours refining this to be visually perfect, so we should not give in unless we have exhausted other options.

Peter - I assume here the other option would be to adjust the canvas size to be smaller? It looks like we have a slightly larger image than needed. I think we can probably cut some of the bottom off of those images. I just wanted to check with you to see if that was a valid fix.

Comment 39

[GitHub Autolander Bot]

Attachment: [gaia] howareyou322:bug1127170 > mozilla-b2g:master

Comment 40

[Peter Chang[:pchang]]

Comment on attachment 8590596 [details] [review]
[gaia] howareyou322:bug1127170 > mozilla-b2g:master

With this patch, I didn't see the visual difference and confirmed there was no overlap between icon and text.

Comment 41

[Kevin Grandon :kgrandon]

Comment on attachment 8590596 [details] [review]
[gaia] howareyou322:bug1127170 > mozilla-b2g:master

I left a comment on github and there is a linter error. Please address and re-flag me. Thanks!

Comment 42

[Peter Chang[:pchang]]

Comment on attachment 8590596 [details] [review]
[gaia] howareyou322:bug1127170 > mozilla-b2g:master

Update to address reviewer's suggestion

Comment 43

[Kevin Grandon :kgrandon]

Comment on attachment 8590596 [details] [review]
[gaia] howareyou322:bug1127170 > mozilla-b2g:master

Seems to work well for me. Thanks.

Comment 44

[Peter Chang[:pchang]]

(In reply to Kevin Grandon :kgrandon from comment #43)
> Comment on attachment 8590596 [details] [review]
> [gaia] howareyou322:bug1127170 > mozilla-b2g:master
> 
> Seems to work well for me. Thanks.

Thanks, kevin.

Comment 45

[Peter Chang[:pchang]]

The try result is positive.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=451fcba532dc

Comment 46

[Peter Chang[:pchang]]

Please help to land the gaia and gecko patches.

Comment 47

[Peter Chang[:pchang]]

There is another gecko patch(attachment 8581447 [details] [diff] [review]) need to be landed

And the try result is in comment 45

Comment 48

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #47)
> There is another gecko patch(attachment 8581447 [details] [diff] [review])
> need to be landed
> 
> And the try result is in comment 45

Add checkin-needed back.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3ecb7edda780

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ecb7edda780