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

RESOLVED FIXED in Firefox 40

Status

defect
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: pchang, Assigned: pchang)

Tracking

unspecified
2.2 S10 (17apr)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:+, firefox40 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → pchang
Blocks: gfxperf
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
(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
(Assignee)

Comment 3

4 years ago
(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.
Can you get a profile of this happening?
Flags: needinfo?(pchang)
(Assignee)

Comment 5

4 years ago
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
Flags: needinfo?(pchang)
(Assignee)

Comment 7

4 years ago
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
(Assignee)

Comment 8

4 years ago
Now I'm using gecko profiler to check the display list since gecko profiler provides the display list visualization.
(Assignee)

Comment 9

4 years ago
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
(Assignee)

Comment 10

4 years ago
(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.
(Assignee)

Updated

4 years ago
Depends on: 1137109
(Assignee)

Updated

4 years ago
No longer depends on: 1137109
(Assignee)

Updated

4 years ago
Depends on: 1137109
(Assignee)

Comment 11

4 years ago
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.
(Assignee)

Comment 12

4 years ago
(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
(Assignee)

Comment 13

4 years ago
Attachment #8580496 - Flags: review?(matt.woodrow)
(Assignee)

Comment 15

4 years ago
This bug requires the attachment 8580496 [details] [diff] [review] and 8580497 to work.
(Assignee)

Comment 16

4 years ago
(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.
(Assignee)

Comment 17

4 years ago
Could you have someone to help on the patch?
(Assignee)

Updated

4 years ago
Flags: needinfo?(gduan)
(Assignee)

Comment 18

4 years ago
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 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.
Attachment #8580496 - Flags: review?(matt.woodrow) → review-
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.
(Assignee)

Comment 21

4 years ago
(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.
(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.
(Assignee)

Comment 23

4 years ago
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
Attachment #8580496 - Attachment is obsolete: true
Attachment #8580543 - Flags: review?(matt.woodrow)
(Assignee)

Comment 24

4 years ago
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 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?
Attachment #8580543 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 26

4 years ago
Update debug log based on reviewer suggestion
Attachment #8580543 - Attachment is obsolete: true
Attachment #8581447 - Flags: review+
(Assignee)

Comment 27

4 years ago
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.
Attachment #8580497 - Attachment is obsolete: true
Flags: needinfo?(gduan)
Attachment #8581467 - Flags: feedback?(sfoster)
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?
Attachment #8581722 - Flags: ui-review?(epang)
Comment on attachment 8581467 [details] [diff] [review]
gaia patch to increase the margin-top to 88px

LGTM, but lets hear from Eric.
Attachment #8581467 - Flags: feedback?(sfoster) → feedback+
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!
Attachment #8581722 - Flags: ui-review?(epang) → ui-review?(pla)
(Assignee)

Comment 31

4 years ago
(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?
Flags: needinfo?(mtsai)

Comment 32

4 years ago
Hi Harly,
Please sync with Peter and see how we an help. Thank you.
Flags: needinfo?(mtsai) → needinfo?(hhsu)

Updated

4 years ago
tracking-b2g: --- → +
Priority: -- → P2

Comment 33

4 years ago
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!
Attachment #8581722 - Flags: ui-review?(pla) → ui-review+
Peter, can you create a github pull request for attachment #8581467 [details] [diff] [review] and ask for review from kgrandon?
Flags: needinfo?(pchang)
Thank you both Peter for providing the background and for doing the quick review :)
Flags: needinfo?(hhsu)
(Assignee)

Comment 36

4 years ago
Create a pull request for gaia changes.
Attachment #8581467 - Attachment is obsolete: true
Flags: needinfo?(pchang)
Attachment #8586516 - Flags: review?(sfoster)
(Assignee)

Updated

4 years ago
Attachment #8586516 - Flags: ui-review+
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
Attachment #8586516 - Flags: review?(sfoster)
Attachment #8586516 - Flags: review?(kgrandon)
Attachment #8586516 - Flags: feedback+
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.
Flags: needinfo?(pchang)
Attachment #8586516 - Flags: review?(kgrandon) → review-
(Assignee)

Comment 40

4 years ago
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.
Flags: needinfo?(pchang)
Attachment #8590596 - Flags: review?(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!
Attachment #8590596 - Flags: review?(kgrandon)
(Assignee)

Comment 42

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

Update to address reviewer's suggestion
Attachment #8590596 - Flags: review?(kgrandon)
Comment on attachment 8590596 [details] [review]
[gaia] howareyou322:bug1127170 > mozilla-b2g:master

Seems to work well for me. Thanks.
Attachment #8590596 - Flags: review?(kgrandon) → review+
(Assignee)

Comment 44

4 years ago
(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.
(Assignee)

Updated

4 years ago
Attachment #8586516 - Attachment is obsolete: true
(Assignee)

Comment 46

4 years ago
Please help to land the gaia and gecko patches.
Keywords: checkin-needed
(Assignee)

Comment 47

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

And the try result is in comment 45
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Attachment #8590596 - Attachment is obsolete: true
(Assignee)

Comment 48

4 years ago
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3ecb7edda780
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S10 (17apr)
You need to log in before you can comment on or make changes to this bug.