Closed Bug 1029718 Opened 6 years ago Closed 6 years ago

5.01% win8 tsvgx regression on mozilla inbound (fx33) June 19th from rev d004e867f67a

Categories

(Core :: Layout, defect)

x86
Windows 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jmaher, Assigned: tnikkel)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression])

Attachments

(1 file)

we have a svg regression that appears to be windows 8 only:
http://graphs.mozilla.org/graph.html#tests=[[281,131,31]]&sel=1403153338000,1403326138000&displayrange=7&datatype=running

I did some retriggers and we go from ~211 -> ~222:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&fromchange=ac426472ceec&tochange=8bc5711c589e&jobname=WINNT%206.2%20mozilla-inbound%20talos%20svgr

Here is some information about the tsvgx test:
https://wiki.mozilla.org/Buildbot/Talos/Tests#tsvg.2C_tsvgx

You should be able to test a fix locally or via try server.
One step ahead of you ;) already investigating this.
thanks!  Glad to see you taking action on the automated alerts, you make me happy :)
Attached patch patchSplinter Review
try confirms this fixes it.
Assignee: nobody → tnikkel
Attachment #8447588 - Flags: review?(mstange)
Comment on attachment 8447588 [details] [diff] [review]
patch

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

Should we also add a check to ScaleRegionToOutsidePixels? I suppose the place that now turns empty regions into non-empty ones is this one: http://hg.mozilla.org/mozilla-central/annotate/2f5df65e3662/layout/base/FrameLayerBuilder.cpp#l2045
Attachment #8447588 - Flags: review?(mstange) → review+
ScaleTo(?)Pixels just calls the request function on the region directly. ScaleToNearestPixels and ScaleToOutsidePixels just use a region iterator in a while loop, so they return an empty region when the given region is empty (it has 0 rects). ScaleToInsidePixels is specialized, but it also returns an empty return for 0 rect regions. So empty regions should stay empty with ScaleTo(?)Pixels.
Thanks for fixing this!
(In reply to Timothy Nikkel (:tn) from comment #5)
> so they return an empty region when the given region is empty
> (it has 0 rects). ScaleToInsidePixels is specialized, but it also returns an
> empty return for 0 rect regions. So empty regions should stay empty with
> ScaleTo(?)Pixels.

Oh good. So you didn't actually regress bug 1016525, at least not in the sense of re-adding expensive region unioning operations. So what was the perf problem here? Just the operation of transforming an empty rect?
Just getting the transform matrix for the frame and transforming it I guess. We should probably expose functions on nsLayoutUtils to get the matrix once and then do the transforming several times.
https://hg.mozilla.org/mozilla-central/rev/2c0234a7ba1e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.