1491997 - Talos sessionrestore regressions caused by Screenshots bootstrap removal

Status: RESOLVED WONTFIX

(Firefox :: Screenshots, defect)

Description

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

A test export of Screenshots without bootstrap[1] seems to have significantly regressed the sessionrestore Talos tests[2]:

sessionrestore opt e10s stylo:
   9.37% osx-10-10
  14.20% windows10-64
  11.98% windows7-32

sessionrestore_no_auto_restore opt e10s stylo:
   6.35% osx-10-10
  10.17% windows10-64
   9.31% windows7-32

Are these regressions acceptable?


[1] https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=199375120&revision=a49b519604998dc7aa9644924ad30d83ec19fdc7

[2] https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=a49b519604998dc7aa9644924ad30d83ec19fdc7&framework=1&selectedTimeRange=172800

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Kris, any thoughts on these regressions?

Comment 2

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Pushed a couple new try runs with no-artifact:

base commit from latest m-c: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f0c522878663e5732f5e8c32489f098108b0d63

bootstrap removal commit, with some bug fixes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=848d71d92e8069f47a72b74291343af84b106df6

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Let's try the bootstrap removal commit again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2231487faec10496991314ffac002f973bfe23d0

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Seeing additional Talos regressions on another Try run: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=1fd026f05d673e46786f0b48deab88c923b5b90e&framework=1&showOnlyImportant=1&selectedTimeRange=172800

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: image.png

FWIW, Friday's Try run, with the Screenshots bootstrap removal applied on top of the fix for bug 1492990, does not resolve the sessionrestore regressions:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=3b072ff9ce513e1bf1a948840b737fe897441e44&framework=1&showOnlyImportant=1&selectedTimeRange=172800

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I'm not sure how to debug these failures. Any ideas?

Comment 7

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

At :aswan's request, I applied patch https://phabricator.services.mozilla.com/D6571 from bug 1492963 on top of the Screenshots bootstrap export, and kicked off a couple of Talos runs:

1) Talos run with profiles attached, `./mach try fuzzy --talos-profile --no-artifact`:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2199f54e2403dd7f3691ddc9a133e1a6d30da23

2) Talos run without profiles, `./mach try -b do -p linux,linux64,macosx64,win32,win64 -u all -t all --rebuild-talos 5 --no-artifact`:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd3e62e6bcb9deffa8d40ebabf3daa878199a651

Comment 8

[Andrew Swan [:aswan]]

The run from comment 7 doesn't actually include profiles (I have no idea why).  Here's my attempt to do a (limited) run with profiles, I'll check in on it tomorrow:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=820f1543b0e61fd5a9ab2daf7960620c4e097864

Comment 9

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Looks like the sessionrestore regressions found in bug 1483172 are quite similar to what's going on here.

I wonder if the regressions here are those earlier regressions, but improved by some performance work that's landed since.

Comment 10

[David Durst [:ddurst]]

(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #9)
> Looks like the sessionrestore regressions found in bug 1483172 are quite
> similar to what's going on here.
> 
> I wonder if the regressions here are those earlier regressions, but improved
> by some performance work that's landed since.

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1483172#c7, I was wondering if applying the patch backed out in https://bugzilla.mozilla.org/show_bug.cgi?id=1483430 makes the regression worse or stays the same. If it's the same, then I'd say this is the new normal (which is starting to be addressed here).

If that's the case, then I'd suggest we close this and focus on bug 1483172.

Comment 11

[Florian Quèze [:florian]]

Do we know if these are actual regressions or just a side effect of changing the startup order? In bug 1492519 comment 6 I looked a bit at the sessionrestore talos test, and I'm not convinced it's measuring the right thing. I think if instead of measuring from sessionRestoreInit we measured from the process creation, the measures would be more useful.

Comment 12

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

> Do we know if these are actual regressions or just a side effect of changing the startup order?

Good question. Maybe :kmag has thoughts.

One odd thing about these regressions is that the sessionrestore test uses the 'sessionstore-windows-restored' event as its end marker[1] (via [2]), but the webextension shouldn't even be initialized until after 'sessionstore-windows-restored' has fired[3].
 
[1] https://searchfox.org/mozilla-central/source/testing/talos/talos/startup_test/sessionrestore/addon/bootstrap.js#90
[2] https://searchfox.org/mozilla-central/source/toolkit/components/startup/nsAppStartup.cpp#721
[3] https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2230-2231

Comment 13

[Thomas Wisniewski [:twisniewski]]

For what it's worth, I'm seeing similar Talos results in bug 1451485, where we're converting the webcompat reporter to a webextension: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=7c96f8096e2e99de60b858b2e6c8b85a70962122&framework=1&showOnlyImportant=1&selectedTimeRange=172800

Comment 14

[David Durst [:ddurst]]

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1483172#c10 and #c11, I suggest wontfixing this bug unless something is exacerbated by bug 1492519.

Comment 15

[Andrew Swan [:aswan]]

I don't know if it will eliminate the regression completely but there's a quick-and-dirty patch on bug 1498027 that might help a little bit.  That patch needs to be cleaned up and tested before landing, but I think it would be fair to do a try run with that patch to see where we'll end up when the real thing lands.

Comment 16

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Closing based on comment 14