Closed Bug 1303625 Opened 4 years ago Closed 4 years ago

73.19 - 75.69% a11yr (windows7-32) regression on push b65ad36d7311 (Fri Sep 16 2016)

Categories

(Firefox :: Untriaged, defect)

51 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
e10s + ---
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: jmaher, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf, regression, talos-regression)

Talos has detected a Firefox performance regression from push b65ad36d7311. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

 76%  a11yr summary windows7-32 opt e10s     4746.37 -> 8338.94
 73%  a11yr summary windows7-32 pgo e10s     4453.02 -> 7712.27


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

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 Talos jobs in a pushlog format.

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

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling

I am not sure if this is more work related to bug 1296571, or if this is different.
:aklotz, can you take a look at this and help determine if this is expected and when we will get a chance to look at windows e10s a11y regressions (or fix up the test case)
Flags: needinfo?(aklotz)
tracking-e10s: --- → ?
(In reply to Joel Maher ( :jmaher) from comment #1)
> :aklotz, can you take a look at this and help determine if this is expected
> and when we will get a chance to look at windows e10s a11y regressions (or
> fix up the test case)

(change was backed out since this was filed, but...)

Had this patch stuck, the plan was to immediately work on performance mitigations. We really need to get a build out to a11y client vendors for testing, perf warts and all. This patch was the last serious correctness issue that we were aware of.

I'm going to investigate the Win8 timeouts because that is suggesting a hang, but once that is dealth with I'd like to get this back in ASAP. Once it is back in, I can move on to performance work. I already have other patches in the queue that drop the a11y score by ~930 points. There are more to follow.
Flags: needinfo?(aklotz)
(In reply to Aaron Klotz [:aklotz] from comment #2)
> Had this patch stuck, the plan was to immediately work on performance
> mitigations. We really need to get a build out to a11y client vendors for
> testing, perf warts and all. This patch was the last serious correctness
> issue that we were aware of.

This sounds like a high priority target indeed, at which case I guess some could argue that it has higher value than avoiding the perf regression - for now.

Assuming there are no other functional issues (like the win8 timeouts), IMO it would be valid to accept the regressions, especially since it seems to affect only the subject which the fixes address (a11y), so other areas seem unaffected, perf wise.

With tradeoffs of correctness vs performance on a specific subject, typically correctness should triumph IMO.

Though I'd prefer to have a second opinion on this, first by aklotz, and then maybe also by someone else, maybe from the a11y team? and/or bsmedberg?
My goal for this project (shared with the a11y team) is to minimize the perf regressions. Of course, going from non-e10s to e10s will entail some regression just by virtue of the fact that there is now IPC. We want it to be as small as possible, however, and we consider the regressions in both this bug and bug 1296571 to be unacceptable.

OTOH, since we're working with third parties who implement a11y clients that interact with us, we need to get correct builds out to them urgently so that we can deal with any e10s compatibility issues with those clients as soon as possible.

a11y on e10s is still disabled by default even though we run full test suites on them during continuous integration. It has been frustrating to me to have things backed out for breaking a disabled, incomplete feature, to the point where I am seriously considering temporarily disabling a11y tests to reflect that. This is also less than ideal because (obviously) those tests do find problems across various Windows releases, so it is a bit of a catch-22.

Having said that, the Win8 Talos timeout is suggesting a hang which I do consider to be a correctness problem as much as a perf problem, so I am holding off on disabling tests or relanding the patch in question until I have answers on that.
See Also: → 1296571
(In reply to Aaron Klotz [:aklotz] from comment #4)
> a11y on e10s is still disabled by default even though we run full test
> suites on them during continuous integration. It has been frustrating to me
> to have things backed out for breaking a disabled, incomplete feature, to
> the point where I am seriously considering temporarily disabling a11y tests
> to reflect that.

This is absolutely debatable, and IMO patches which only regress a configuration which is not shipped should not be backed out, for the reasons you named.

Obviously, before it's shipped, the numbers should be revisited, but as long as it doesn't ship, I see no harm in keeping the patches in tree.
I have a new alert for tracking the regressions:
== Change summary for alert #3300 (as of September 19 2016 13:35 UTC) ==

Regressions:

 50%  a11yr summary windows8-64 opt e10s     5144.38 -> 7726.37
 46%  a11yr summary windows7-32 pgo e10s     5279.53 -> 7705.2
 22%  a11yr summary windows7-32 opt e10s     6836.18 -> 8369.65

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=3300
Blocks: 1302124
No longer blocks: 1291351
Depends on: 1304449
Hi :jmaher,
Do we mark 51 as won't fix
Flags: needinfo?(jmaher)
bug 1304449 is the fix for this. When it lands on 52 I'd like to request uplift, which of course then puts the decision in relman's hands ;-)
Flags: needinfo?(jmaher)
Blocks: 1258839
a11yr scores are back down to previous levels on inbound and central. Resolving.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Mark 51 fixed as bug 1304449 was fixed in 51.
You need to log in before you can comment on or make changes to this bug.