Last Comment Bug 806256 - 3D CSS Periodic table demo is unusably slow in Firefox
: 3D CSS Periodic table demo is unusably slow in Firefox
Status: VERIFIED FIXED
[sps]
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla20
Assigned To: Matt Woodrow (:mattwoodrow)
:
: Jet Villegas (:jet)
Mentors:
: 806295 807252 (view as bug list)
Depends on: 808838 509052 807563 815040 815666
Blocks: 725936 815351
  Show dependency treegraph
 
Reported: 2012-10-28 16:59 PDT by Benoit Girard (:BenWa)
Modified: 2013-01-31 07:33 PST (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't invalidate when the overflow changes (2.87 KB, patch)
2012-10-28 17:42 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Don't invalidate when the overflow changes v2 (2.20 KB, patch)
2012-10-28 17:43 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
Don't overwrite the initial overflow (3.15 KB, patch)
2012-10-29 22:22 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
Only recompute preserve-3d/perspective children overflow areas if the frame size changed (4.78 KB, patch)
2012-10-29 22:24 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
Walk through nsDisplayWrapList without flushing the temp list (4.76 KB, patch)
2012-10-29 22:26 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
WIP: Cache the result of ReadTransforms (10.70 KB, patch)
2012-10-29 22:30 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
WIP: Only recompute child transforms if they are dependent on the size (4.13 KB, patch)
2012-10-29 22:32 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review

Comment 1 Benoit Girard (:BenWa) 2012-10-28 17:08:05 PDT
Flush styles takes about 250ms a frame and then the paints are about 350ms.
Comment 2 Matt Woodrow (:mattwoodrow) 2012-10-28 17:42:02 PDT
Created attachment 676015 [details] [diff] [review]
Don't invalidate when the overflow changes

Pretty sure this is safe, and reduces some of the unnecessary painting.

Looks like the biggest chunk of time is being spent in ComputePreserve3DChildrenOverflow (~60%), most of which is in GetResultingTransformMatrix.
Comment 3 Matt Woodrow (:mattwoodrow) 2012-10-28 17:43:26 PDT
Created attachment 676016 [details] [diff] [review]
Don't invalidate when the overflow changes v2

Removed accidental printf code.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-10-28 18:22:54 PDT
Comment on attachment 676016 [details] [diff] [review]
Don't invalidate when the overflow changes v2

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

Eep, I forgot we still had this.

Removing this block means you can simplify the code further. Get rid of hasClipPropClip, didHaveClipPropClip, hasOutlineOrEffects and preTransformVisualOverflowChanged.
Comment 5 Anthony Ricaud (:rik) 2012-10-29 15:43:25 PDT
*** Bug 806295 has been marked as a duplicate of this bug. ***
Comment 6 Matt Woodrow (:mattwoodrow) 2012-10-29 22:22:54 PDT
Created attachment 676477 [details] [diff] [review]
Don't overwrite the initial overflow

Not really a big performance issue, but this bug meant that we overwrote the initial overflow with the transformed one. Was causing us to allocate much larger layers than we needed.
Comment 7 Matt Woodrow (:mattwoodrow) 2012-10-29 22:24:19 PDT
Created attachment 676479 [details] [diff] [review]
Only recompute preserve-3d/perspective children overflow areas if the frame size changed

This is a huge performance gain in this demo, ComputePreserve3DChildrenOverflow was more than 50% of the profile.
Comment 8 Matt Woodrow (:mattwoodrow) 2012-10-29 22:26:38 PDT
Created attachment 676480 [details] [diff] [review]
Walk through nsDisplayWrapList without flushing the temp list

Previously we would flush the temporary list (and wrap them in an nsDisplayTransform) when we encountered an nsDisplayWrap list. In the case where we had multiple nsDisplayWrapLists, we'd end up with multiple nsDisplayTransforms.

This changes the behaviour so we walk through nsDisplayWrapList, it's about a 2/3 reduction in the number of nsDisplayTransform/ContainerLayers created in this demo.

I think there are still bugs here.
Comment 9 Matt Woodrow (:mattwoodrow) 2012-10-29 22:30:42 PDT
Created attachment 676483 [details] [diff] [review]
WIP: Cache the result of ReadTransforms

This is no longer useful for this demo, since the previous patches stop us from recomputing so many transforms.

For the record, the basic problem with the demo was that it had a single preserve-3d parent with a few hundred transformed children. Each frame of the animation changes the transform on each child.

For each child transform change, we walk up the ancestor tree calling UpdateOverflow(), and this hits ComputePreserve3DChildrenOverflow, which computes the transform for all the children. O(n^2) behaviour is not fun.

This patch might be useful for other cases where we do end up computing transforms too often, but I don't want to add this complexity unnecessarily.
Comment 10 Matt Woodrow (:mattwoodrow) 2012-10-29 22:32:46 PDT
Created attachment 676484 [details] [diff] [review]
WIP: Only recompute child transforms if they are dependent on the size

As above, no longer useful on this demo.

Might be useful if we find a use-case that is changing the size of the preserve-3d parent frame, but the transform doesn't actually depend on the size.
Comment 11 Matt Woodrow (:mattwoodrow) 2012-10-29 22:35:10 PDT
Note that we still have the issue that changing the overflow of multiple children results in us calling UpdateOverflow() on the common ancestors multiple times. Much less of an issue now that the O(n^2) path is gone, but it might be nice if we could coalesce these.
Comment 12 Matt Woodrow (:mattwoodrow) 2012-10-31 13:50:50 PDT
*** Bug 807252 has been marked as a duplicate of this bug. ***
Comment 13 Chris Lord [:cwiiis] 2012-10-31 17:13:02 PDT
Editing the bug title slightly so that searching for 'periodic table' gets this.
Comment 15 Oleg Romashin (:romaxa) 2012-10-31 20:37:23 PDT
without this fix windows 8 require actual reboot after opening this page, would make sense to increase priority
Comment 17 Benoit Girard (:BenWa) 2012-11-05 08:09:40 PST
Verified on 2012-11-04)
Comment 18 Benoit Girard (:BenWa) 2012-11-05 16:22:20 PST
Here's a profile of what I get with these patches. It's running well with OGL but not well with Basic:
http://people.mozilla.com/~bgirard/cleopatra/#report=f63355043dd883c2243f5a06a7359fc797f7be3e

New bug to track further work needed: bug 808838
Comment 19 Matt Woodrow (:mattwoodrow) 2013-01-29 15:33:37 PST
Backed out from beta: https://hg.mozilla.org/releases/mozilla-beta/rev/dc5dec1293ab
Comment 20 Mihai Morar, (:MihaiMorar) 2013-01-31 07:33:18 PST
I confirm fix is verified on FF 19b4 and Aurora Latest (2013-01-30) on Mac OS 10.8 x64. Dependency chain with: Bug 807563 and Bug 815666.

Note You need to log in before you can comment on or make changes to this bug.