Closed Bug 985017 Opened 6 years ago Closed 6 years ago

10% svgx regression on b2g-inbound on march 7th for osx 10.8 and win 8 platforms

Categories

(Core :: ImageLib, defect, P1, major)

defect

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.4+
Tracking Status
firefox29 --- unaffected
firefox30 + fixed
firefox31 + fixed
b2g-v1.3 --- unaffected
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: jmaher, Assigned: mwu)

References

Details

(Keywords: perf, regression, Whiteboard: [c= p= s= u=1.4] [talos_regression][qa-])

Attachments

(2 files, 1 obsolete file)

10.8:
http://graphs.mozilla.org/graph.html#tests=[[281,201,24]]&sel=none&displayrange=30&datatype=running

win8:
http://graphs.mozilla.org/graph.html#tests=[[281,203,31]]&sel=none&displayrange=30&datatype=running

these both show a jump in the steady stream of numbers starting with the landing of bug 962670.  I am not sure if this is related to svg, but it doesn't look like a far stretch.

I see hixie-004.xml as taking a permanent hit from this push while many other tests take a more noisy approach:
https://datazilla.mozilla.org/?start=1394018401&stop=1394591478&product=Firefox&repository=B2G-Inbound-Non-PGO&os=win&os_version=6.2.9200&test=tsvgx&graph_search=2429468e82dd&tr_id=4745823&graph=hixie-004.xml&project=talos

in fact you can see the original push, the backout and the repush.
Keywords: perf, regression
Whiteboard: [talos_regression]
Assignee: nobody → mwu
Depends on: 962670
Turns out creating gfxImageSurfaces can be very expensive. This patch creates a gfxImageSurface for the drawing path that isn't thrown out. However, it also doesn't hold a lock on the volatile buffer, so the drawing path needs to grab a lock before using this surface.

Going to run on try before requesting review. However, this appears to fix the regression locally on my mac mini.
thanks for looking into this, this affects aurora and is seen on graph server!
This makes it easier to put VolatileBufferPtr in the right scope (see next patch for better context). mPurged is set to false, but it really has no meaning when we're working with a null VolatileBufferPtr. What it does do is make things fail faster if the code didn't consider the possibility of VolatileBufferPtr being null.
Attachment #8395892 - Attachment is obsolete: true
Attachment #8396106 - Flags: review?(mh+mozilla)
This patch creates a gfxImageSurface/gfxQuartzImageSurface so the drawing path doesn't have to create a new one on every draw. In exchange, the drawing path needs to lock the volatile buffer manually in order to use this cached surface.
Attachment #8396107 - Flags: review?(seth)
this same regression from hg.mozilla.org/integration/b2g-inbound/rev/965fe5b195ab seems to be causing 10.8 and win8 tp5 regressions as well.  Looking forward to this landing!
Please make sure to get this uplifted on 1.3T,central and aurora to resolve it on all branches. I'll sync-up with sheriffs if needed
blocking-b2g: 1.4? → 1.4+
Severity: normal → major
Priority: -- → P1
Whiteboard: [talos_regression] → [c= p= s= u=1.4] [talos_regression]
Attachment #8396106 - Flags: review?(mh+mozilla) → review+
are we going to uplift this to Aurora?
Comment on attachment 8396107 [details] [diff] [review]
[2/2] Cache a gfxImageSurface for the drawing path

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

Looks good.
Attachment #8396107 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/d7fcc0c1a744
https://hg.mozilla.org/mozilla-central/rev/2c4d1aa4e976
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
this is resolved fixed, but we have a regression on aurora, are we going to land there or let this go?
(In reply to Joel Maher (:jmaher) from comment #13)
> this is resolved fixed, but we have a regression on aurora, are we going to
> land there or let this go?

See comment 8.
thanks!
Depends on: 991767
No longer depends on: 991767
Depends on: 993066
No longer depends on: 993066
Depends on: 1005081
Whiteboard: [c= p= s= u=1.4] [talos_regression] → [c= p= s= u=1.4] [talos_regression][qa-]
You need to log in before you can comment on or make changes to this bug.