1415467 - Export hybrid version of follow-on search add-on to mozilla-central

Status: RESOLVED WONTFIX

(Firefox :: Search, enhancement, P3)

Description

[Mark Banner (:standard8)]

Once mozilla-central is 59, we should export the latest version of the follow-on search add-on from github, so that we can start getting the WebExtension version tested.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1415467 - Update follow-on search to v1.0.0; Switch parts of it to be a WebExtension.

Review commit: https://reviewboard.mozilla.org/r/198042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198042/

Comment 2

[Mark Banner (:standard8)]

I won't push this until next week, once 58 has branched.

The only changes here are to switch the main parts of the add-on to a WebExtension, overall it is now classified as a hybrid extension.

https://github.com/mozilla/followonsearch/releases/tag/v1.0.0

For reference, we've already done Talos pushes as documented here https://github.com/mozilla/followonsearch/pull/21, they all appeared to be green with no significant regressions.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8926788 [details]
Bug 1415467 - Update follow-on search to v1.0.0; Switch parts of it to be a WebExtension.

https://reviewboard.mozilla.org/r/198042/#review203348

I'm getting this error when running with this patch:

1510238892288	addons.xpi-utils	WARN	addMetadata: Add-on [e-mail] is invalid: Error: Error while loading 'file:///Users/past/src/gecko/obj-artifact/dist/Nightly.app/Contents/Resources/browser/features/[e-mail]/webextension/manifest.json' (NS_ERROR_FILE_NOT_FOUND) (resource://gre/modules/Extension.jsm:439:18) JS Stack trace: readJSON/</<@Extension.jsm:439:18 < [e-mail]:131:17 < [e-mail]:936:3 < [e-mail]:1188:21 < [e-mail]:1535:23 < [e-mail]:3332:7 < async*[e-mail]:3097:12 < [e-mail]:57:28 < async*[e-mail]:53:5 < async*[e-mail]:1035:5 < [e-mail]:345:9 < [e-mail]:1183:9 < onBeforeBrowserWindowShown/<@SessionStore.jsm:1330:9 < promise callback*[e-mail]:1315:5 < [e-mail]:769:9 < [e-mail]:1281:5 < [e-mail]:1:1

Looking at screenshots, I believe you need to add something like this to moz.build:

FINAL_TARGET_FILES.features['[e-mail]']["webextension"] += [
  'webextension/background.js',
  'webextension/manifest.json'
]

Comment 4

[Mark Banner (:standard8)]

I'd completely forgotten to ensure it was working correctly when exported. You're right they need fixing, there's also a few other things that need fixing for running as a built-in add-on (though I'm not sure why we didn't see those from the repo).

Also, this means the talos runs I did were invalid, so I'll get a new set up and running.

Comment 5

[Mark Banner (:standard8)]

Baseline Talos comparison with the startup fixed:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f1aec8d82d924247c9f538c64b2cf69f983b4c7d&newProject=try&newRevision=cddebce9c0dce9dc0b57a1f56783121e29356ceb&framework=1

Talos comparison with additional potential startup speed fix:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f1aec8d82d924247c9f538c64b2cf69f983b4c7d&newProject=try&newRevision=9b1944b75ed7f6ae3ab675a71e4b924a7636a2b6&framework=1

No results yet, posting to ensure I don't loose them.

Comment 6

[Mark Banner (:standard8)]

Update on Talos:

- We definitely need the additional startup speed fix - without it sessionrestore and ts_paint take big hits.

- With the speed fix, there still looks to be regressions around "tp5o responsiveness" and "tp5o opt". I'm guessing due to having the WebExtension run the background script.

I'll take a look at those next week sometime.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Thank you for the thorough investigation!

Comment 8

[Mark Banner (:standard8)]

Joel, we're trying to convert the existing follow-on search add-on to a WebExtension. Talos on try is currently showing big regressions with tp5o and a few others. Who can help us understand what might be causing these, and how they can be fixed?

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f1aec8d82d924247c9f538c64b2cf69f983b4c7d&newProject=try&newRevision=9b1944b75ed7f6ae3ab675a71e4b924a7636a2b6&framework=1&showOnlyImportant=1

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

it seems that whenever we look at webextensions the same tests end up with perf issues.  I am happy to see startup issues resolved here, now to figure out responsiveness and tp5o.

What is interesting is we have tp5o* and ts_paint run in parallel with a web-extension for testing purposes.  For tp5o summary there is a 1-2% regression with this change when comparing the web extension data, but looking a the original tp5 test without web extensions, we see a 4-5% regression.  responsiveness is about twice as bad 8 vs 16%.

A few questions that come to mind:
1) why would removing the search addon and replacing it with a web-extension yield a regression- it should be minimal
2) theoretically the webext tests would have any perf hit taken, so we shouldn't see regressions with future web-extensions
3) maybe web extensions have a few big perf bugs to be found- our webextension that we test with is very basic.
4) do other web extensions exhibit this type of perf regression

I recall :kmag and :jhirsch doing some great work with screenshots to fix perf regressions- possibly they have some ideas?

Comment 10

[Mark Banner (:standard8)]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #9)
> A few questions that come to mind:
> 1) why would removing the search addon and replacing it with a web-extension
> yield a regression- it should be minimal

The only thing I can think of here, is potentially to do with taking the existing frame script [0] and replacing it with a web extension background script [1].

The script is basically doing the same thing in both instances - monitoring page loads so we can look at the urls loaded.

[0] https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/browser/extensions/followonsearch/content/followonsearch-fs.js

[1] https://github.com/mozilla/followonsearch/blob/ceed297b8287de98ba7c641470f082f47a24ed2d/add-on/webextension/background.js

Comment 11

[Andy McKay]

Kris, could you help on comment 9 please?

Comment 12

[Kris Maglione [:kmag]]

I can't say in detail without seeing a profile. There are a few things going on, though:

1) The most serious regressions are on Linux, where WebExtensions still run in the main process.

2) The background page uses a tab update listener. Tab update listeners unfortunately fire a lot, and are relatively expensive. Most of the cost is per-listener, serializing the tab information and dispatching the message, so that overhead would still show up on top of the existing tab listener overhead in the tp5o_webext test.

3) The tab listener itself is relatively expensive. It does lots of linear array searches, inside a for-of loop, for every https URL load. It would be better to just generate a map of all domains and do a single lookup.

Comment 13

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

FWIW, Screenshots delays startup significantly to avoid impacting startup performance:
- the webextension isn't started until the 'sessionstore-windows-restored' event fires[1]
- the bulk of the background page code isn't loaded until the UI is clicked by the user[2]

The follow-on search code doesn't seem like it absolutely _must_ run at startup, so this kind of workaround might be something to consider, though it pushes you back into legacy territory (bootstrap + embedded webextension).

Also, Screenshots has a bug filed to investigate removing the delays[3].

[1] https://github.com/mozilla-services/screenshots/blob/master/addon/bootstrap.js#L54
[2] https://github.com/mozilla-services/screenshots/blob/master/addon/webextension/background/startBackground.js
[3] https://github.com/mozilla-services/screenshots/issues/3160

Comment 14

[Mark Banner (:standard8)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #12)
> I can't say in detail without seeing a profile. There are a few things going
> on, though:

I've done a talos push with a profile here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bc5eb0faad85e465eff38e6a3e3d73500e33596

Hopefully that helps.

> 1) The most serious regressions are on Linux, where WebExtensions still run
> in the main process.
> 
> 2) The background page uses a tab update listener. Tab update listeners
> unfortunately fire a lot, and are relatively expensive. Most of the cost is
> per-listener, serializing the tab information and dispatching the message,
> so that overhead would still show up on top of the existing tab listener
> overhead in the tp5o_webext test.

I did a stripped down version of the WebExtension, which was just adding the most simple listener, with that I think it can be seen that most of this is definitely the WebExtension api.

With the full listener:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f1aec8d82d924247c9f538c64b2cf69f983b4c7d&newProject=try&newRevision=9b1944b75ed7f6ae3ab675a71e4b924a7636a2b6&framework=1&filter=tp5o&showOnlyImportant=1

tp5o responsiveness opt e10s
linux-qr 16.47%
windows-10-64 29.95%

With the stripped-down listener:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=cd5ded5a7f7d1fa79d75ad09291504c135b42bdf&newProject=try&newRevision=f8bad11202c425f9657568f2c67fb338dcdcfc24&framework=1

tp5o responsiveness opt e10s
linux-qr 13.45%
windows-10-64 9.06%

> 3) The tab listener itself is relatively expensive. It does lots of linear
> array searches, inside a for-of loop, for every https URL load. It would be
> better to just generate a map of all domains and do a single lookup.

I believe that normally the for-of loop wouldn't be invoked for most URL loads - there's a statement that checks to see if it is a query URI and returns early. The tp5o urls don't seem to be query-based, though given the differences with the stripped-down listener, there's obviously something going on here.

However, I have started working on a version that uses maps for the lookup and I'll see if how that compares with the other versions as well.

Comment 15

[Mark Banner (:standard8)]

My attempted performance improvement to use maps is here: https://hg.mozilla.org/try/rev/8fe6230e3bee036a5c03f994df4b479b490b947d

Here's the comparison of central to the current WebExtension version:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=cd5ded5a7f7d1fa79d75ad09291504c135b42bdf&newProject=try&newRevision=8b7f447795db3f0ffad9e05c60a00feb702eee23&framework=1

and here's the version with the revised performance improvement:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=cd5ded5a7f7d1fa79d75ad09291504c135b42bdf&newProject=try&newRevision=89c81add38afd8a7371622498baf40524faeec21&framework=1

This indicates there may be a slight improvement on Linux, but Windows is a lot worse (tp5o responsiveness opt e10s goes from 6% regression to 12% regression).

Although, the weird thing is, if we take a look at the comparisons with the "empty" listener baseline:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=cd5ded5a7f7d1fa79d75ad09291504c135b42bdf&newProject=try&newRevision=f8bad11202c425f9657568f2c67fb338dcdcfc24&framework=1

The empty listener is a 9% tp5o responsiveness opt e10s regression, but the original WebExtension is a 6%.

Which is bringing some doubt into the reliability of those numbers I think. Hence, I'm not quite sure what the best way forward is here.

Comment 16

[Mark Banner (:standard8)]

I've moved the WebExtensions performance issue to bug 1433879.

Comment 17

[Mark Banner (:standard8)]

Blocked by WebExtensions performance, so unassigning & dropping down to a P3.

Comment 18

[Mark Banner (:standard8)]

Wontfixing in favour of bug 1475571, we're looking at moving this in-product.