1404126 - followonsearch-fs takes significant amount time when opening a new tab

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Olli Pettay [:smaug][[e-mail]]]

I was trying to profile youtube page load again.
(STR, open some in-process page, type https://www.youtube.com/ and press enter)
I noticed something I haven't seen before in the profiles, followonsearch-fs
taking significant amount of new tab time, surprisingly lot comparing to other message manager scripts.
Looks like it is forcing about:blank creation, which forces some stylesheet loading and what not.
Could we not do all that?

https://perfht.ml/2fUGsM9

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

It is very possible that some of that slowness moves elsewhere when the addon is disabled. I'll reprofile after disabling it.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Except that I don't know how to disable it. I thought it was part of Search Shield Study but apparently not.

Comment 3

[Mark Banner (:standard8)]

The follow-on search add-on is a system add-on.

The only thing the frame script should be doing is adding a couple of listeners watching for page load / progress. There's no document creation or anything.

At a glance, the only thing I can see that could be an issue is that we're saving the content variable in the global scope when the script starts:

http://searchfox.org/mozilla-central/rev/20e41d4a61a8f5e34c9cf357304b78b3e9bced8a/browser/extensions/followonsearch/content/followonsearch-fs.js#274

I wonder if that's somehow triggering the document creation which maybe would happen somewhere else if the frame script hadn't triggered it?

Reviewing the code again, we don't actually need to save the content variable in the global scope, so we should see if that helps here or not.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Yes, accessing the 'content' triggers about:blank creation, I think.

Comment 5

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/followonsearch/pull/18

Comment 6

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

Mark don't forget to land the update soon in m-c in order to get it into beta. It would be nice (but not critical) to avoid having to use SAO delivery for this after 57 is released.

Comment 7

[Florian Quèze [:florian]]

[Tracking Requested - why for this release]: comment 6 sounds like we want this in 57. The patch seems low risk and the perf benefit significant.

Comment 8

[Mark Banner (:standard8)]

Attachment: Bug 1404126 - Upgrade Follow-on search add-on to 0.9.5.

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

Comment 9

[Mark Banner (:standard8)]

The update I've just attached includes the perf fix for the add-on and a few other fixes.

https://github.com/mozilla/followonsearch/releases/tag/v0.9.5

    Fix issues during upgrade/install of add-on.
    Fix an issue accessing the content scope too early which adversely affects tab open performance.
    Tag searches on Google without codes.
    Add some more Bing codes to match against.

Comment 10

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

Comment on attachment 8915958 [details]
Bug 1404126 - Upgrade Follow-on search add-on to 0.9.5.

https://reviewboard.mozilla.org/r/187234/#review192240

LGTM

Comment 11

[Marcia Knous [:marcia]]

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> [Tracking Requested - why for this release]: comment 6 sounds like we want
> this in 57. The patch seems low risk and the perf benefit significant.

Tracking 57+ for the perf win here, especially with low risk.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f0a242f6396
Upgrade Follow-on search add-on to 0.9.5. r=past

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6f0a242f6396

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Is this something you're looking to uplift to 57?

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8915958 [details]
Bug 1404126 - Upgrade Follow-on search add-on to 0.9.5.

Approval Request Comment
[Feature/Bug causing the regression]: Follow-on search
[User impact if declined]: Small performance decrease sometimes when opening a new tab. See also comment 9 for additional fixes as part of this upgrade.
[Is this code covered by automated tests?]: No, we've been doing manual testing.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: QA already have steps for testing the add-on, xref bug 1394083.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple, small well-scoped changes that only affect the add-on itself.
[String changes made/needed]: None

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8915958 [details]
Bug 1404126 - Upgrade Follow-on search add-on to 0.9.5.

Must fix, beta57+

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Smaug, could you please verify this bug is fixed as expected on a latest Nightly build? Thanks!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d055ef797ede

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Hmm, this is trickier to reproduce since we handle child processes different after bug 1385249.
I guess I should try beta.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Oh, beta has older version of that.
Nightly has 0.9.5, and the overhead it adds per process load is apparently around 0.5m.
(which isn't perhaps too accurate, but at least I don't see anything too bad in the profile)