1465524 - 3.74% Images (osx-10-10) regression on push fd067216c3a6ef2ff0620e25c2261186a8fdc714 (Tue May 29 2018)

Status: RESOLVED WONTFIX

(Firefox :: Theme, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

We have detected an awsy regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=fd067216c3a6ef2ff0620e25c2261186a8fdc714

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  4%  Images osx-10-10 opt stylo     7,213,895.36 -> 7,483,887.92


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=13547

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/AWSY/Tests

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

:ntim, I see you authored the patch in bug 1464699, can you look into this regression and see if you can fix it?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Joel, I don't see anything in the patch that can be changed to limit this regression. Switching from PNG to SVG will introduce more work and it doesn't seem avoidable. Can we acknowledge this regression and accept it?

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

thanks :jaws for looking at this.  I am glad to know we have a logical explanation for the increase.  while I am personally fine with this, lets let :erahm make the final call.

Comment 4

[Daniel Holbert [:dholbert]]

It looks like there regression is, more specifically, a 14.29% regression in each of the "Images Fresh start [+30s] opt stylo" and "Images Fresh start opt stylo" subtests:

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=autoland&originalRevision=b2d0891dd19c037cb66d72f0736e3a902f44adbb&newProject=autoland&newRevision=fd067216c3a6ef2ff0620e25c2261186a8fdc714&originalSignature=74e3d669d81dd7c536d410b754f1ef58fce6dac1&newSignature=74e3d669d81dd7c536d410b754f1ef58fce6dac1&framework=4

CC'ing some other SVG/imagelib folks who might have thoughts or at least be interested being aware of this sort of regression caused by switching to SVG images.

Comment 5

[Markus Stange [:mstange]]

Do we have before/after about:memory dumps from these tests? Does the measured value only include the memory taken up by the pixels in the image surfaces, or also the size of the SVG document that gets created for such an image? Do we know how many cached pixel surfaces we have for a given SVG image?

Comment 6

[Eric Rahm [:erahm]]

(In reply to Markus Stange [:mstange] from comment #5)
> Do we have before/after about:memory dumps from these tests? Does the
> measured value only include the memory taken up by the pixels in the image
> surfaces, or also the size of the SVG document that gets created for such an
> image? Do we know how many cached pixel surfaces we have for a given SVG
> image?

For start settled we're seeing ~100K regression. It looks like this is mainly 'unfiledBookmarks.svg' and 'bookmarksToolbar.svg'. 50K for a 16x16 svg seems a bit excessive.

> ├──0.09 MB (39.45%) -- images/chrome
> │  ├──0.10 MB (41.24%) -- vector/used
> │  │  ├──0.05 MB (20.80%) -- image(16x16, chrome://browser/skin/places/unfiledBookmarks.svg)
> │  │  │  ├──0.05 MB (20.27%) ── source [+]
> │  │  │  └──0.00 MB (00.53%) ── locked/surface(16x16)/decoded-heap
> │  │  └──0.05 MB (20.44%) -- image(16x16, chrome://browser/skin/places/bookmarksToolbar.svg)
> │  │     ├──0.05 MB (19.92%) ── source [+]
> │  │     └──0.00 MB (00.53%) ── locked/surface(16x16)/decoded-heap
> │  └──-0.00 MB (-1.79%) -- raster/used/<non-notable images>
> │     ├──-0.00 MB (-1.05%) ── decoded-heap
> │     └──-0.00 MB (-0.74%) ── source

Comment 7

[Markus Stange [:mstange]]

Ah ok, so 50K is the size of each SVG document then, I think.
https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/image/VectorImage.cpp#431

IIRC jwatt has done some investigation into SVG document sizes in the past, maybe he has some insights here.

Comment 8

[Tim Nguyen :ntim]

I don't think I have anything to add from what everyone else said here.

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Markus Stange [:mstange] from comment #7)
> Ah ok, so 50K is the size of each SVG document then

Yeah, that's in line with bug 999931 comment 14. We could reduce that further with work, but bug 1353771 is possibly a better approach (but note bug 1353771 comment 11). At any rate, there's no quick fix here.

Comment 10

[Ionuț Goldan [:igoldan]]

(In reply to Jonathan Watt [:jwatt] from comment #9)
> (In reply to Markus Stange [:mstange] from comment #7)
> > Ah ok, so 50K is the size of each SVG document then
> 
> Yeah, that's in line with bug 999931 comment 14. We could reduce that
> further with work, but bug 1353771 is possibly a better approach (but note
> bug 1353771 comment 11). At any rate, there's no quick fix here.

Can you please link all bugs that can lessen/fix our perf regression? I'm referring to bugs like bug 1353771.

Comment 11

[Ionuț Goldan [:igoldan]]

(In reply to Jonathan Watt [:jwatt] from comment #9)
> (In reply to Markus Stange [:mstange] from comment #7)
> > Ah ok, so 50K is the size of each SVG document then
> 
> Yeah, that's in line with bug 999931 comment 14. We could reduce that
> further with work, but bug 1353771 is possibly a better approach (but note
> bug 1353771 comment 11). At any rate, there's no quick fix here.

Can you provide a rough estimate?

Comment 12

[Jonathan Watt [:jwatt]]

Estimate of time? That work is not on any roadmap, so unless it's bumped up the priority list by someone I don't see anyone working on it this year.

Comment 13

[Jim Mathies [:jimm]]

Based on comments from engineers familiar with what caused this memory regression we're going to take this.