3D CSS Periodic table demo is unusably slow in Firefox

VERIFIED FIXED in mozilla20

Status

()

Core
Layout
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: BenWa, Assigned: mattwoodrow)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla20
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sps])

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
STR:
http://mrdoob.com/lab/javascript/threejs/css3d/periodictable/

Run fast on on chrome.

Performance profile:
http://people.mozilla.com/~bgirard/cleopatra/?report=85e0f281078aab53f0b20988c2cd6400b1ff37a7
(Reporter)

Comment 1

5 years ago
Flush styles takes about 250ms a frame and then the paints are about 350ms.
Component: Graphics: Layers → Layout
(Assignee)

Comment 2

5 years ago
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.
Attachment #676015 - Flags: review?(roc)
(Assignee)

Comment 3

5 years ago
Created attachment 676016 [details] [diff] [review]
Don't invalidate when the overflow changes v2

Removed accidental printf code.
Attachment #676015 - Attachment is obsolete: true
Attachment #676015 - Flags: review?(roc)
Attachment #676016 - Flags: review?(roc)
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.
Attachment #676016 - Flags: review?(roc) → review+

Updated

5 years ago
Duplicate of this bug: 806295
Whiteboard: [sps]
(Assignee)

Comment 6

5 years ago
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.
Attachment #676477 - Flags: review?(roc)
(Assignee)

Comment 7

5 years ago
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.
Attachment #676479 - Flags: review?(roc)
(Assignee)

Comment 8

5 years ago
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.
Attachment #676480 - Flags: review?(roc)
(Assignee)

Comment 9

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

Comment 10

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

Comment 11

5 years ago
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.
Attachment #676477 - Flags: review?(roc) → review+
Attachment #676479 - Flags: review?(roc) → review+
Attachment #676480 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Depends on: 509052
(Assignee)

Updated

5 years ago
Duplicate of this bug: 807252
Blocks: 725936
Editing the bug title slightly so that searching for 'periodic table' gets this.
Summary: 3D CSS Periodic demo is unusably slow in Firefox → 3D CSS Periodic table demo is unusably slow in Firefox
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/af535a8e7f6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/374270441a49
https://hg.mozilla.org/integration/mozilla-inbound/rev/08d397c13da6
https://hg.mozilla.org/integration/mozilla-inbound/rev/01123067aa58
(Assignee)

Updated

5 years ago
Assignee: nobody → matt.woodrow
without this fix windows 8 require actual reboot after opening this page, would make sense to increase priority
Depends on: 807563
https://hg.mozilla.org/mozilla-central/rev/af535a8e7f6e
https://hg.mozilla.org/mozilla-central/rev/374270441a49
https://hg.mozilla.org/mozilla-central/rev/08d397c13da6
https://hg.mozilla.org/mozilla-central/rev/01123067aa58
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Reporter)

Comment 17

5 years ago
Verified on 2012-11-04)
Status: RESOLVED → VERIFIED
(Reporter)

Comment 18

5 years ago
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

Updated

5 years ago
Depends on: 808838

Updated

5 years ago
Depends on: 815040

Updated

5 years ago
Depends on: 815666
Blocks: 815351
(Assignee)

Comment 19

4 years ago
Backed out from beta: https://hg.mozilla.org/releases/mozilla-beta/rev/dc5dec1293ab

Updated

4 years ago
Target Milestone: mozilla19 → mozilla20
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.
You need to log in before you can comment on or make changes to this bug.