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

Status: RESOLVED WORKSFORME

(Core :: SVG, defect)

Description

[timbugzilla]

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.

Comment 1

[Benjamin Smedberg]

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.

Comment 2

[timbugzilla]

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

Comment 3

[timbugzilla]

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)

Comment 4

[timbugzilla]

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

[Benjamin Smedberg]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The total time spend doing busy is 61% of the profile.

7% is BuildDisplayList.
16% is BuildContainerLayerFor.
32% is actual painting.

Comment 7

[Matt Woodrow (:mattwoodrow)]

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

Comment 8

[Matt Woodrow (:mattwoodrow)]

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

Bas, is there anything we can do here?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Matt Woodrow (:mattwoodrow)]

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

[Benjamin Smedberg]

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?

Comment 12

[Jonathan Watt [:jwatt]]

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.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: 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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 16

[Robert Longson [:longsonr]]

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.

Comment 17

[Jonathan Watt [:jwatt]]

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.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't build transforms for translation-only shifts v2

Comment 19

[Jonathan Watt [: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.

Comment 20

[Jonathan Watt [:jwatt]]

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.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't build transforms for translation-only shifts v3

Comment 22

[Jonathan Watt [:jwatt]]

Comment on attachment 715291 [details] [diff] [review]
Don't build transforms for translation-only shifts v3

Looks great, thanks, Matt!

Comment 23

[Jonathan Watt [:jwatt]]

Matt, is your patch here good to land?

Comment 24

[Matt Woodrow (:mattwoodrow)]

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

I hope so, it passed try.

Comment 25

[Matt Woodrow (:mattwoodrow)]

Apparently not, caused reftest failures.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/24460dad116e

Comment 26

[Jonathan Watt [:jwatt]]

Bug 869505 would also help here, given that currently more time is spent processing display list items than actually painting (just).

Comment 27

[Jonathan Watt [:jwatt]]

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

Comment 28

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add ApplyTransform for flatenning nsDisplayTransform items

Comment 29

[Matt Woodrow (:mattwoodrow)]

Attachment: 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.

Comment 30

[Ciprian Muresan [:cmuresan], Ecosystem QA]

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