Very slow loading and scrolling of page with SVG infographics at nytimes.com

RESOLVED WORKSFORME

Status

()

RESOLVED WORKSFORME
6 years ago
3 years ago

People

(Reporter: timbugzilla, Assigned: mattwoodrow)

Tracking

(Blocks: 2 bugs, {perf, testcase-wanted})

Trunk
x86
Windows 8
perf, testcase-wanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [in-the-wild], URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Slow loading and scrolling, with jank and sometimes hangs, when loading and scrolling this infographic page from the NY Times website. 

Infographics are rendered using SVG/js.

HWA (D2D & D3D10) is on.
(Reporter)

Updated

6 years ago
Component: General → SVG
Product: Firefox → Core

Comment 1

6 years ago
This is important to me because the NYT is experimenting with SVG instead of canvas and I want to make sure that we get the experience right as quickly as possible. Is there somebody who can profile to figure out what we're actually doing?

timbugzilla, would you be willing to run a nightly build and grab a profile using the builtin profiler? https://developer.mozilla.org/en-US/docs/Performance/Profiling_with_the_Built-in_Profiler has instructions.
Flags: needinfo?
(Reporter)

Comment 2

6 years ago
Thanks for the response. I used B. Girard's gecko profiler extension and grabbed the following profile for page loading:

http://people.mozilla.com/~bgirard/cleopatra/#report=b9b3b8bbe5f91222508c44cc6aab331b24b829b1
Flags: needinfo?
(Reporter)

Comment 3

6 years ago
This profile is for scrolling the page from top to bottom:

http://people.mozilla.com/~bgirard/cleopatra/#report=137cfffc07141acc15cb50dbbf63befc33cb1bf2

(all profiles were taken with today's moz-central win32 nightly build)
(Reporter)

Comment 4

6 years ago
Those profiles were taken on an AMD E-350 based laptop. The following profiles were taken on a faster desktop machine (Intel E-8500 CPU with AMD HD-4650 GPU, win7 x64).

Page loading:

http://people.mozilla.com/~bgirard/cleopatra/#report=e48f2686699089aa171e243dec694011015cda40

Page scrolling from top to bottom:

http://people.mozilla.com/~bgirard/cleopatra/#report=0110b8c67eb19d799967e517baaaf895c5d57b73

Comment 5

6 years ago
Sorry I meant "SVG instead of Flash" in comment 1

I'm not an expert in our layers/graphics code, but the scrolling profile from comment 3 appears to be almost entirely painting: we're in nsLayoutUtils::PaintFrame alternating between nsDisplayList::PaintRoot and nsLayoutUtils::PaintFrame::BuildDisplayList

The loading jank appears to be slow JS from the nytimes website.
The total time spend doing busy is 61% of the profile.

7% is BuildDisplayList.
16% is BuildContainerLayerFor.
32% is actual painting.
(Assignee)

Comment 7

6 years ago
Paint flashing doesn't show any obviously unnecessary painting, so this is just us being slow at painting the large amount of content.
(Assignee)

Comment 8

6 years ago
Interestingly, most of the painting time is spent inside DrawTargetD2D::FlushTransformToRT.

Bas, is there anything we can do here?
There seemed to be quite a bit of painting under PaintInactiveLayer. I think it's worth investigating the display list and layer trees to make sure they're not unnecessarily complex.
(Assignee)

Comment 10

6 years ago
The display list is (unsurprisingly) massive: http://people.mozilla.org/~mwoodrow/display-list.txt

There are a lot of transforms there that only contain a translation, and wrap a single child. I wonder if we could skip building a layer in that case and just offset the painting of the child.

Comment 11

6 years ago
That's a very common pattern in generated SVG content which uses placed sprites. If we could optimize that case it might help. timbugzilla or anyone, are you willing to save/create an offline testcase for this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(timbugzilla)
Keywords: testcase-wanted
Dunno why they didn't just use the cx/cy attribute. :/ Definitely not the first time I've seen content using a translation transform instead of the more appropriate attributes though.

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> There are a lot of transforms there that only contain a translation, and
> wrap a single child. I wonder if we could skip building a layer in that case
> and just offset the painting of the child.

I think in the case of SVG leaf frames that would make sense most of the time.
(Assignee)

Comment 13

6 years ago
Created attachment 710041 [details] [diff] [review]
Don't build transforms for translation-only shifts

I don't love this, I'd much rather have a more generic solution.

Unfortunately any attempts to strip out nsDisplayTransform items during visibility computation/ layer builder would require us to adjust the bounds of all the child items. I can't think of any nice way to implement that.

It does work for this page though, and approximately doubls scrolling fps.
Attachment #710041 - Flags: review?(jwatt)
Attachment #710041 - Flags: feedback?(roc)
Comment on attachment 710041 [details] [diff] [review]
Don't build transforms for translation-only shifts

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

This seems reasonable.
Attachment #710041 - Flags: feedback?(roc) → feedback+
Comment on attachment 710041 [details] [diff] [review]
Don't build transforms for translation-only shifts

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

Actually I think it would be better if this were folded into nsSVGPathGeometryFrame::GeneratePath. That would avoid bugs where we forget to apply the translation.
Attachment #710041 - Flags: feedback+ → feedback-
Do we need similar changes to nsSVGForeignObjectFrame and nsSVGDisplayContainerFrame too? It looks like these 3 classes contain some very similar code for handling transforms that it would be nice to factor out.
(Reporter)

Updated

6 years ago
Flags: needinfo?(timbugzilla)
Comment on attachment 710041 [details] [diff] [review]
Don't build transforms for translation-only shifts

Canceling review request until comment 15 is addressed.

(In reply to Robert Longson from comment #16)
> Do we need similar changes to nsSVGForeignObjectFrame and
> nsSVGDisplayContainerFrame too? It looks like these 3 classes contain some
> very similar code for handling transforms that it would be nice to factor
> out.

It's not so clear that we want to avoid the creation of layers in the case of containers.
Attachment #710041 - Flags: review?(jwatt)
(Assignee)

Comment 18

6 years ago
Created attachment 712322 [details] [diff] [review]
Don't build transforms for translation-only shifts v2
Attachment #710041 - Attachment is obsolete: true
Attachment #712322 - Flags: review?(jwatt)
Comment on attachment 712322 [details] [diff] [review]
Don't build transforms for translation-only shifts v2

To anyone looking at nsSVGPathGeometryFrame::BuildDisplayList the GetTranslation call isn't going to make much sense, and in fact even the implementation of GetTranslation doesn't really make much sense without understanding the subtleties of what nsSVGPathGeometryFrame::IsSVGTransformed would now do.

How about making nsSVGPathGeometryFrame::IsSVGTransformed do:

  if (content->GetAnimatedTransformList() ||
      content->GetAnimateMotionTransform()) {
    gfxMatrix temp = content->PrependLocalTransformsTo(gfxMatrix(),
                       nsSVGElement::eUserSpaceToParent);
    if (temp.HasNonTranslation()) {
      if (aOwnTransform) {
        *aOwnTransform = temp;
      }
      foundTransform = true;
    }
  }

And renaming GetTranslation to GetOwnTransformIfOnlyTranslation, and changing what it does to match its name. That way we'd always translate the context in GeneratePath if we have a translation only transform even if we've created an nsDisplayTransform due to the parent having children-only transforms, but the code becomes a lot easier to explain and understand.
And at some point in the future we're hoping to have separate nsDisplayTransforms for the parent's children-only transform and for childrens' own transforms anyway.
(Assignee)

Comment 21

6 years ago
Created attachment 715291 [details] [diff] [review]
Don't build transforms for translation-only shifts v3
Attachment #712322 - Attachment is obsolete: true
Attachment #712322 - Flags: review?(jwatt)
Attachment #715291 - Flags: review?(jwatt)
Summary: Very slow loading and scrolling of page with SVG inforgraphics at nytimes.com → Very slow loading and scrolling of page with SVG infographics at nytimes.com
Comment on attachment 715291 [details] [diff] [review]
Don't build transforms for translation-only shifts v3

Looks great, thanks, Matt!
Attachment #715291 - Flags: review?(jwatt) → review+
Matt, is your patch here good to land?
Flags: needinfo?(matt.woodrow)

Updated

6 years ago
Keywords: perf
(Assignee)

Comment 24

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6b34be6fb4c

I hope so, it passed try.
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 25

6 years ago
Apparently not, caused reftest failures.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/24460dad116e
Bug 869505 would also help here, given that currently more time is spent processing display list items than actually painting (just).
Depends on: 869505

Updated

5 years ago
Blocks: 869505
No longer depends on: 869505
We didn't make a note of what failed last time we pushed this, so I unbitrotted this patch and pushed it to Try to see what fails:

https://tbpl.mozilla.org/?tree=Try&rev=82a9ec79fe44

Updated

5 years ago
Whiteboard: [in-the-wild]
(Assignee)

Updated

5 years ago
Assignee: nobody → matt.woodrow
(Assignee)

Updated

5 years ago
Depends on: 965030
(Assignee)

Comment 28

5 years ago
Created attachment 8367773 [details] [diff] [review]
Part 1: Add ApplyTransform for flatenning nsDisplayTransform items
(Assignee)

Comment 29

5 years ago
Created attachment 8367775 [details] [diff] [review]
Part 2: Implement ApplyTransform for nsDisplaySVGPathGeometry

Still a couple of test failures, both because we now snap to the nearest app unit.

One of them is a failing assertion because the cached bounds of an existing nsDisplayList no longer matches the bounds of the items.

This is because we are translating by a partial app unit, and the bounds of nsDisplayTransform is computed in gfx pixels, and then rounded to the nearest whole app unit (which affects the size, not just the position). The new code just rounds the translation to app units, which doesn't affect the size.

Still thinking about how to fix this.
Attachment #715291 - Attachment is obsolete: true
Blocks: 1013297
Hi,

Build ID    20160210153822
User Agent  Mozilla/5.0 (Windows NT 6.2; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID    20160224030246
User Agent  Mozilla/5.0 (Windows NT 6.2; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0

I have tested this issue on the latest Firefox release (44.0.2) and latest Nightly (47.0a1) and the nytimes site no longer causes Firefox to run slow or to have difficulties with scrolling. As such I will close this issue as Resolved-Worksforme. If you run into this problem in the future, please feel free to reopen this bug.

Thanks,
Cipri
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.