Closed Bug 1067588 Opened 5 years ago Closed 5 years ago

3-20% Win7|8 regressions on inbound (v.35) Sept 12th from push c232720a2847

Categories

(Core :: Graphics, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jmaher, Assigned: mattwoodrow)

References

Details

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

Attachments

(2 files)

a big set of regressions came in late last week:
+---------------------+----------------+-------------------------------+---------+-----+--------+
| date                | platform       | test                          | percent | bug | status |
+---------------------+----------------+-------------------------------+---------+-----+--------+
| 2014-09-12 16:42:31 | WINNT 6.1 (ix) | TP5 Scroll                    | -10.6%  |     |        |
| 2014-09-12 16:43:28 | WINNT 6.1 (ix) | Customization Animation Tests | -21.6%  |     |        |
| 2014-09-12 16:44:37 | WINNT 6.1 (ix) | SVG-ASAP                      | -12.7%  |     |        |
| 2014-09-12 16:43:54 | WINNT 6.1 (ix) | Tab Animation Test            | -10.6%  |     |        |
| 2014-09-12 16:46:41 | WINNT 6.1 (ix) | Tp5 Optimized                 | -3.26%  |     |        |
| 2014-09-12 16:47:09 | WINNT 6.1 (ix) | TResize                       | -20%    |     |        |
| 2014-09-12 16:47:13 | WINNT 6.2 x64  | TResize                       | -17.6%  |     |        |
| 2014-09-12 17:06:27 | WINNT 6.2 x64  | Tp5 Optimized                 | -2.62%  |     |        |
| 2014-09-12 17:23:07 | WINNT 6.2 x64  | Customization Animation Tests | -13.2%  |     |        |
| 2014-09-12 17:23:32 | WINNT 6.2 x64  | Tab Animation Test            | -9.03%  |     |        |
| 2014-09-12 17:24:13 | WINNT 6.2 x64  | SVG-ASAP                      | -12.7%  |     |        |
+---------------------+----------------+-------------------------------+---------+-----+--------+

looking on graph server, you can see that this is the case:
http://graphs.mozilla.org/graph.html#tests=[[254,131,25]]&sel=1410492849802,1410530649802&displayrange=7&datatype=running

We don't have a scenario here where we didn't build or run tests for a few builds, this is the real deal and here is the push that caused the regression:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c232720a2847

It appears :mattwoodrow is on PTO for this week, we need to get somebody else to look at this.
:bas, can you comment on this as to what was expected, and what we can do to reduce these regressions?
Flags: needinfo?(bas)
(In reply to Joel Maher (:jmaher) from comment #1)
> :bas, can you comment on this as to what was expected, and what we can do to
> reduce these regressions?

With Matt on PTO it's my opinion we should probably back out the integration patches. But JWatt is the best person to chime in.
Flags: needinfo?(bas) → needinfo?(jwatt)
I agree with Bas. (bug 1044702 comment 13 also suspects it of causing crashes.)
Flags: needinfo?(jwatt)
OK, who's doing the backout?
Flags: needinfo?(jwatt)
Flags: needinfo?(bas)
I'm back now!

I've got a fix up for the crash, will look at the perf stuff this week too.
Flags: needinfo?(jwatt)
Flags: needinfo?(bas)
Assignee: nobody → matt.woodrow
Attachment #8496062 - Flags: review?(bas)
Attachment #8496062 - Flags: review?(bas) → review+
This shows improvements, in fact the TART, CART, and TResize tests are back to normal.  Tp5 is fixed from this regression (although other regressions make the graph look bad, but for this bug it is fixed!)

The svg tests are still regressed:
win7 svgx: http://graphs.mozilla.org/graph.html#tests=%5B%5B281,131,25%5D%5D&sel=none&displayrange=30&datatype=running
win8 svgx: http://graphs.mozilla.org/graph.html#tests=%5B%5B281,131,31%5D%5D&sel=none&displayrange=30&datatype=running
https://hg.mozilla.org/mozilla-central/rev/689bcb752bf3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
I would like to leave this open until we have agreement on what we are doing with the svgx regressions on windows.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Are there multiple regressions in those graphs?

For svgx on windows 7 it looks like we were averaging around 238, then we jumped up to ~253 briefly before jumping again to ~277 (maybe with yet another step in the middle).

I have a patch on try that looks like it'll get us back to ~253, not sure if that covers all the regressions that I caused or not.
If the subimage covers the entire image then we don't want to enforce the sampling restriction.

This moves the code around to benefit from the checks around CSRD instead of having our own ones.
Attachment #8497938 - Flags: review?(bas)
the try push shows tsvgx having an improvement :)
Comment on attachment 8497938 [details] [diff] [review]
Don't extract a subimage if it would be the entire image

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

Hrm, does this not reduce the cases in which we use the DrawWithSamplingRect function?
(In reply to Bas Schouten (:bas.schouten) from comment #16)
> Comment on attachment 8497938 [details] [diff] [review]
> Don't extract a subimage if it would be the entire image
> 
> Review of attachment 8497938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hrm, does this not reduce the cases in which we use the DrawWithSamplingRect
> function?

Yes, it should skip it if the sampling rect matches the image rect.
Attachment #8497938 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/f4f1c6516ec0
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
this cleared up the win7 regression, but not the win8:
http://graphs.mozilla.org/graph.html#tests=%5B%5B281,131,31%5D%5D&sel=none&displayrange=7&datatype=running

:bas, should we leave this alone, it looks like we have resolved all but one issue!
Flags: needinfo?(bas)
(In reply to Joel Maher (:jmaher) from comment #20)
> this cleared up the win7 regression, but not the win8:
> http://graphs.mozilla.org/graph.html#tests=%5B%5B281,131,
> 31%5D%5D&sel=none&displayrange=7&datatype=running
> 
> :bas, should we leave this alone, it looks like we have resolved all but one
> issue!

Probably still good to look at, although Win8 should now be using D2D 1.1. We should probably open a separate bug.
Flags: needinfo?(bas)
(In reply to Joel Maher (:jmaher) from comment #20)
> :bas, should we leave this alone, it looks like we have resolved all but one
> issue!

Joel, can you open a new bug for whatever issue you're referring to here and note the number in this bug?

(In reply to Joel Maher (:jmaher) from comment #22)
> this has caused a 7.3% win8 tp5o_scroll regression:

Also can you clarify what you mean by "this"?
Flags: needinfo?(jmaher)
the tp5 scroll regression appears to be fixed, I believe there is no pending regressions from this bug:
http://graphs.mozilla.org/graph.html#tests=%5B%5B323,63,31%5D,%5B323,131,31%5D%5D&sel=none&displayrange=90&datatype=running
Flags: needinfo?(jmaher)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.