Closed Bug 1362412 Opened 7 years ago Closed 7 years ago

foofighters.com is still slow to animate because of unnecessary invalidation for non-border-collapse tables

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Project Flags:
Performance Impact medium
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: bkelly, Assigned: ethlin)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: perf)

Attachments

(3 files)

Archive.zip
10.11 KB, application/zip
Details
test.html
637 bytes, text/html
Details
Bug 1362412 - Do not invalidate table cell's parent frame if it's not border collapse or there is no border.
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details 
In bug 1342854 we discovered that foofighters.com does a "train board" style animation that runs quite slower in FF than chrome.  We determined this was caused by two issues:

1) We had some layout perf issues related to tables.  (bug 929484)
2) The animation was running via an open-loop setTimeout() mechanism.  The timers were being delayed by the anti-timer-flood changes since the main thread was busy with table layout (as caused by the previous issue).

In bug 1342854 we mitigated this by allowing some timers to run consecutively.  This brought foofighters.com close to parity with chrome, but not quite.

Since, bug 929484 has also been fixed.  (Should be in 5/6/2017 nightly.)

Something must have changed on the site, though, because neither of these solutions is working any more.

I can tell that the timer mitigation mechanism is still working because I can change the pref and see it effect the output of:

https://mozdevs.github.io/servo-experiments/experiments/tiles/

I used a mozilla-inbound build of bug 929484 as well, but that did not seem to help.

We should probably profile this again to see what is happening.
I guess I should mention for triage purposes, though, this site is not well designed.  Last I looked it was running an open-loop animation via setTimeout().  Each flip of a letter in the "train board" is a separate timer.  Its pretty easy for this to back up and clog the main thread.  I don't think this design is that common on the web since it has other issues.
Profile using today's nightly (before bug 929484):

https://perfht.ml/2q7VYeo
14ms consecutively in nsLayoutUtils::PaintFrame::BuildDisplayList
14ms consecutively in mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder *,mozilla::layers::LayerManager *,nsIFrame *,nsDisplayItem *,nsDisplayList *,mozilla::ContainerLayerParameters const &,mozilla::gfx::Matrix4x4Typed<mozilla::gfx::UnknownUnits,mozilla::gfx::UnknownUnits> const *,unsigned int)
21ms consecutively in mozilla::layers::ClientPaintedLayer::PaintThebes(nsTArray<mozilla::layers::ReadbackProcessor::Update> *)
Olli suggests bug 1348469 could help a bit here.
Depends on: 1348469
In general there is just tons of painting.
It'd be nice to see if bug 1331718 helps here, and how much of a win it is in such a paint-heavy page.
(In reply to Olli Pettay [:smaug] from comment #5)
> In general there is just tons of painting.

If I turn on paint flashing it seems some operation is causing us to overpaint the entire table.

1. I get individual flashes for many letters, which is good.
2. But about twice a second we repaint the entire table.  Its unclear why that happens.
This overpainting seems to still happen even with the mozilla-inbound build that includes bug 929484.
Any ideas Matt?
Component: DOM → Layout
Flags: needinfo?(matt.woodrow)
The table code still has 'manual' invalidation (implementations of nsIFrame::InvalidateFrame, and calls to InvalidateFrame) rather than relying on comparison of the display items.

Morris, do we still have cases where tables building display items that paint multiple frames, or are we always building display items for each frame separately now?

If it's the latter, then we can probably remove the table invalidation code, which will hopefully fix the overinvalidation for this page.
Flags: needinfo?(matt.woodrow) → needinfo?(mtseng)
For border collapse, we still create a display item to paint all collapse border. But for other generic part, yes, we build display items for each frame.
Flags: needinfo?(mtseng)
In that case, we could probably make most/all of the explicit table invalidations happen only for border-collapse tables.

Do you want to have a look at doing that Morris once you've finished with the item conversion?
Flags: needinfo?(mtseng)
I can but I cannot do it very soon because I have some other priority items. Anyone interested in it can steal it. :)
Flags: needinfo?(mtseng)
Depends on: 1343912
I prototyped bug 1343912 and it appears to help a lot.  It would still be nice to improve layout perf, though.
Actually, I think the site is changing under us.  Its gotten better in our main release builds again.
Any chance we can get a offline cache of the poorly performing version?

Jet, this might be a relatively easy introduction to display-lists/invalidation if you know of anyone looking? (and depending on what the qf priority ends up as).

For the most part we just want to make the explicit invalidation (nsIFrame::InvalidateFrame) calls within table reflow conditional on the table not being border collapse, and then confirm that things still work.

There's a few possible approaches:

* Use a debugger to find out what's being invalidated on this page, remove those calls, confirm that the page still works (and use paint flashing to confirm that it's invalidating less).

* Work the other way, read the code and make specific test cases for each invalidation call.

* Remove it all and hope! I'm a little worried that our current test coverage for table invalidation is likely to be insufficient, but this change would be very easy to backout (since we're adding a condition, not removing code), so we can let the trains find any issues.

A combination of all 3 might yield the best results.
Flags: needinfo?(bugs)
(In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> Any chance we can get a offline cache of the poorly performing version?

Whatever they changed, its still over-invalidating as described in comment 7.
(In reply to Morris Tseng [:mtseng] [:Morris] from comment #13)
> I can but I cannot do it very soon because I have some other priority items.
> Anyone interested in it can steal it. :)

Thanks, Morris. Happy to steal it from you. Ya-Chieh would give it a try on those approaches mentioned in comment 16.
Assignee: nobody → ywu
Status: NEW → ASSIGNED
Flags: needinfo?(bugs)
qf:p1 because improving table invalidation was deemed important.
Whiteboard: [qf] → [qf:p1]
(In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> For the most part we just want to make the explicit invalidation
> (nsIFrame::InvalidateFrame) calls within table reflow conditional on the
> table not being border collapse, and then confirm that things still work.

We might actually want to condition it on the table not being border-collapse *and* having borders.  But maybe the first bit is a good start.  (And we probably shouldn't create the table border display item if there aren't actually any borders.)  We could perhaps track on the border-collapsed table whether anything in it has a border.
I tried to let debugger stop at 'nsTableFrame::InvalidateTableFrame' after "foofighters.com" starts "train board" style animation but I can't see any calls call to this. Do I miss anything? Any suggestions would be helpful. thank you.
Flags: needinfo?(matt.woodrow)
Looks like they didn't use table anymore as comment 15 said....
I turn on the paint flashing and confirm that the table-like part is indeed still being repainted now. I think I did miss something here before. clear needinfo from Matt Woodrow first. thank you.
Flags: needinfo?(matt.woodrow)
Whiteboard: [qf:p1] → [qf:p2]
Well, foofighters.com has completely redesigned their site.  The train table thing is gone.
Bug 1342854 attached a zip of the page as attachment 8841461 [details] (from ~3 months ago), is it useful for this issue?
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #25)
> Bug 1342854 attached a zip of the page as attachment 8841461 [details] (from
> ~3 months ago), is it useful for this issue?

Yea, if people are still interested in fixing the over-painting issue.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #25)
> Bug 1342854 attached a zip of the page as attachment 8841461 [details] (from
> ~3 months ago), is it useful for this issue?

Thanks, jryans. I haven't noticed there's a snapshot attached in bug 1342854.
Ya-Chieh, could you please double check whether we still have over-painting issue in table frame?
I had tried to use attachment 8841461 [details] in Bug 1342854 but I couldn't see any table style there too. For the table frame's issue that was mentioned on comment 16, we should probably make our own test case but using this website's data. I will do the test case and put them here.
(In reply to Ya-Chieh Wu from comment #28)
> I had tried to use attachment 8841461 [details] in Bug 1342854 but I
> couldn't see any table style there too. For the table frame's issue that was
> mentioned in comment 16, we should probably make our own test case but using
> this website's data. I will do the test case and put them here.

Looks like Bug 1367747 already handles what was mentioned in comment 16 and since I can't see any table style for this website(include the cached attachment), let's keep focusing on div's behavior.
Nope, bug 1367747 only remove some unwanted display item creation. It didn't touch any table invalidation code.
ok! thank you Morris. I will file another bug for this.
See Bug 1370833 for what was mentioned in comment 16.
Attached file Archive.zip — 
As I tried to remove the suspicious changed background, I still can see over painting every few seconds. It is unclear for me what's going on with those <div> so I decided to look into the code of the website, and it wasn't a pleasant journey :( My first step is trying to reduce the complexity of the website, so we can know what's going with those <div> structure. This zip could be able to help to debug if someone is interested. I am putting my efforts on another p1 bug first so I will slowly touch this bug. btw there might have some chances that we didn't over paint at all. It could be the script accidentally changed the part where we thought it did not change.

How to use the zip:
open Foo.html 
turn on painting flash
hit "click"
you can see some part far away from "click" is repainting.
thank you so much for the help, Matt. I will take a look at that.
Attached file test.html — 
This test case is what happened in foofighter.com.  

STR: 
1. turn on the nglayout.debug.paint_flashing
2. open this test.html
3. hit the word "click" on the page

You can see the area we repain is large.
Priority: -- → P2
Keywords: perf
Summary: foofighters.com is still slow to animate → foofighters.com is still slow to animate because of unnecessary invalidation for non-border-collapse tables
Are you still working on this Ya-Chieh?
Flags: needinfo?(ywu)
Sorry that I am not working on this. Also, I think foofighter might have other over invalidation issues but the unnecessary invalidation for non-border-collapse tables. As the attachment, if you turn the paint flash and hit "click me", you can see the repainting area is really large.
Assignee: ywu → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(ywu)
Assignee: nobody → ethlin
Attachment #8925831 - Flags: review?(matt.woodrow)
Comment on attachment 8925831 [details]
Bug 1362412 - Do not invalidate table cell's parent frame if it's not border collapse or there is no border.

https://reviewboard.mozilla.org/r/197022/#review202182
Attachment #8925831 - Flags: review?(matt.woodrow) → review+
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28b9f2407366
Do not invalidate table cell's parent frame if it's not border collapse or there is no border. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/28b9f2407366
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1464928
Performance Impact: --- → P2
Whiteboard: [qf:p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: