Closed Bug 1491997 Opened 6 years ago Closed 6 years ago

Talos sessionrestore regressions caused by Screenshots bootstrap removal

Categories

(Firefox :: Screenshots, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jhirsch, Unassigned)

References

Details

Attachments

(1 file)

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
Kris, any thoughts on these regressions?
Flags: needinfo?(kmaglione+bmo)
Depends on: 1492963
Attached image image.png (deleted) —
I'm not sure how to debug these failures. Any ideas?
Flags: needinfo?(aswan)
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
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
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.
(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.
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.
> 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
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.
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.
Flags: needinfo?(aswan)
Closing based on comment 14
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: