2% Win7 e10s ts_paint, win8 paint regression on fx.team (v.45) on Nov 27, 2015 from push 19876a153a00

RESOLVED INVALID

Status

defect
P2
normal
Rank:
25
RESOLVED INVALID
4 years ago
3 years ago

People

(Reporter: jmaher, Assigned: mikedeboer)

Tracking

({perf, regression})

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-)

Details

(Whiteboard: [talos_regression][btpp-followup-2016-04-25])

Attachments

(1 attachment)

A small ts_paint regression exists from some patches in bug 1223573 which landed on Nov 27th on fx-team.  After some retriggers:
https://treeherder.mozilla.org/#/jobs?repo=fx-team&filter-searchStr=talos%20e10s%20other%20win&fromchange=556ab9e3521b&tochange=d7152cc38e8f

we can see a compareview:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=f48352b311eb&newProject=fx-team&newRevision=19876a153a00&filter=ts_paint&showOnlyConfident=1

these are small regressions and from what I can tell the only real regressions coming from this specific push are the ts_paint ones.  These are small, but seem to be real (despite the large volume of noise- shifting patterns of noise!)
Reporter

Comment 1

4 years ago
Mark, sorry to bother you on so many bugs- finally catching up on all the alerts that have been coming in.  This is small- but we should see if there is something simple to fix this vs ignore it completely.  Maybe this is expected and we can have it documented here in this bug.
Flags: needinfo?(standard8)
Ok, so I'm guessing this is a similar issue to the size bug - there's extra initialisation going on for the add-on. We're also registering style sheets that we weren't before.

I'm not sure which would be the biggest impact - the add-on registration or the style sheets, but I would guess it could be either of them, and I doubt there's much we could do here.
Flags: needinfo?(standard8)
Reporter

Comment 3

4 years ago
is there a quick check we could do to see if it is the registration or the style sheets?  The more we understand the better we can make decisions in the future even if there is nothing to fix for this specific issue.
You could probably just comment it out in the code:

http://hg.mozilla.org/mozilla-central/annotate/5ba77225c957/browser/extensions/loop/bootstrap.js#l800

Perhaps not fully the same, but it might be close enough.
Reporter

Comment 6

4 years ago
ok, commenting out the style sheet code:
https://hg.mozilla.org/try/rev/de7d9e9896c5

didn't do the trick according to compare chooser:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=53859be1a511&newProject=try&newRevision=3155506346cc&showOnlyImportant=0

We have pretty much identical results for the baseline and the commented out version.
Mark, are you tracking this?
Flags: needinfo?(standard8)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #7)
> Mark, are you tracking this?

Roughly. Unfortunately I have very little experience of these perf tests so I've been relying on Joel.

I'm also not sure there is much that we can do about it - but that depends on where the issue is. Taking out the style sheets was my best guess. I guess we could try taking out most of bootstrap.js and see if anything changes.

Also cc'ing other folks who might have experience of this area.
Flags: needinfo?(standard8)
I'll see if I can tease a profile out of this for you.
Flags: needinfo?(mconley)
Reporter

Updated

4 years ago
Summary: 2% Win7 e10s ts_paint regression on fx.team (v.45) on Nov 27, 2015 from push 19876a153a00 → 2% Win7 e10s ts_paint, win8 paint regression on fx.team (v.45) on Nov 27, 2015 from push 19876a153a00
Rank: 19
Priority: -- → P1
Assignee

Comment 10

3 years ago
I'll be driving this bug.

Mike, what kind of profile were you thinking about?
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Bah - sorry I never got to this - holidays happened.

Pushed for profiling to try:

Without bug 1223573 ("good"): https://treeherder.mozilla.org/#/jobs?repo=try&revision=33a9120cc7a9
With bug 1223573 ("bad"): https://treeherder.mozilla.org/#/jobs?repo=try&revision=57eadf5209e7
Flags: needinfo?(mconley)
Bah - and resetting the needinfo on myself to do the analysis once these come in.
Flags: needinfo?(mconley)
This is not e10s only (it seems to affect Win8 64, Linux 64, and Windows XP, all non-e10s), so minusing.
tracking-e10s: --- → -
Rank: 19 → 25
Priority: P1 → P2
Rank: 25 → 19
Priority: P2 → P1
not sure what the two pushes are on try, but ts_paint isn't looking as a regression.
Assignee

Comment 19

3 years ago
(In reply to Joel Maher (:jmaher) from comment #18)
> not sure what the two pushes are on try, but ts_paint isn't looking as a
> regression.

The newRevision commit contains the 'fix' I wanted to test out if it'd resolve the regression. It doesn't, unfortunately.
Rank: 19 → 25
Priority: P1 → P2
If the theory is that this is invoking the CustomizableUI that's at issue, was a similar regression seen when the pocket add-on landed?
Mike, here's a thought: Pocket landed after we did, but uses the same startup flow that we do - create the widget when the add-on's startup() function is called.

Your try push only moved the creating for the widget for Hello, not for Pocket.

Maybe we need to fix both add-ons to fix the regression?

In the event this does fix it, maybe customisable UI could be set to warn/reject if a createWidget or other call comes in before the expected startup?
Flags: needinfo?(mdeboer)
Assignee

Comment 22

3 years ago
Could be true - not sure unless we try!
Flags: needinfo?(mdeboer)
Assignee

Updated

3 years ago
Status: ASSIGNED → NEW
Ed, its possibly getting a bit late for this bug now, but could you take on doing a fix as per my comment 21?

I think Mike can help with which try pushes to do / how to get perf info.
Flags: needinfo?(edilee)
Ed, also ping me or ask questions in the bug, running on try is fairly easy and viewing the results are not too difficult once you know where to look.

Comment 25

3 years ago
Baseline ts_paint:
https://hg.mozilla.org/try/rev/f52121663cedf7a7bc3ca2d53672098f7522e4d1
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f52121663ced
talos-linux64-ix: 1173, 1180, 1179, 1173, 1187 -> 1,178 average
tst-linux64-spot: 2747, 2478, 2802, 2627, 2626 -> 2,656 average

browser-ui-startup-complete ts_paint:
https://hg.mozilla.org/try/rev/44bf96fe66adee013f1417e90e6b4e49a64b94fb
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44bf96fe66ad
talos-linux64-ix: 1180, 1185, 1170, 1183, 1182 -> 1,180 average
tst-linux64-spot: 2715, 2760, 2505, 2272, 2651 -> 2,581 average

Not much change for the first set of machines but roughly 2% for the tst-* machines (although the variance is much higher).

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f52121663ced&newProject=try&newRevision=44bf96fe66ad&framework=7&filter=ts_paint&showOnlyImportant=0
Flags: needinfo?(edilee)
(In reply to Ed Lee :Mardak from comment #25)
> Not much change for the first set of machines but roughly 2% for the tst-*
> machines (although the variance is much higher).

Please just ignore the tst-* results, that's an experiment gone wrong. See bug 1260926.
What's the way forward here now? Or do we close this as won't/can't fix?
Flags: needinfo?(mdeboer)
Flags: needinfo?(mconley)
Flags: needinfo?(jmaher)
Whiteboard: [talos_regression] → [talos_regression][btpp-followup-2016-04-25]
If it's any consolation, I have a fix for a completely unrelated bug that looks like it might give us a nice ts_paint win across the board (bug 1261842), so I don't know how much we should prioritize this investigation, tbh.
Flags: needinfo?(mconley)
Assignee

Updated

3 years ago
Flags: needinfo?(mdeboer)

Comment 30

3 years ago
hello removed so we can close this bug

Comment 31

3 years ago
(In reply to timse201 from comment #30)
> hello removed so we can close this bug

Mark, OOI, was there a perf win when Hello got removed again?
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(standard8)
Resolution: --- → INVALID
Yes ts_paint did improve, although cart regressed for some reason. See bug 1290808.
Depends on: 1290808
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.