Closed
Bug 1404126
Opened 6 years ago
Closed 6 years ago
followonsearch-fs takes significant amount time when opening a new tab
Categories
(Firefox :: Search, defect, P1)
Firefox
Search
Tracking
()
RESOLVED
FIXED
Firefox 58
People
(Reporter: smaug, Assigned: standard8)
Details
(Whiteboard: [fxsearch])
Attachments
(2 files)
49 bytes,
text/x-github-pull-request
|
Details | Review | |
59 bytes,
text/x-review-board-request
|
past
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
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
Reporter | ||
Comment 1•6 years ago
|
||
It is very possible that some of that slowness moves elsewhere when the addon is disabled. I'll reprofile after disabling it.
Reporter | ||
Comment 2•6 years ago
|
||
Except that I don't know how to disable it. I thought it was part of Search Shield Study but apparently not.
Assignee | ||
Comment 3•6 years ago
|
||
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.
Reporter | ||
Comment 4•6 years ago
|
||
Yes, accessing the 'content' triggers about:blank creation, I think.
Comment 5•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → standard8
Status: NEW → ASSIGNED
Priority: -- → P1
Assignee | ||
Updated•6 years ago
|
Whiteboard: [fxsearch]
Comment 6•6 years ago
|
||
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.
Flags: needinfo?(standard8)
Updated•6 years ago
|
Comment 7•6 years ago
|
||
[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-firefox57:
--- → ?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•6 years ago
|
||
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.
Flags: needinfo?(standard8)
Comment 10•6 years ago
|
||
mozreview-review |
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
Attachment #8915958 -
Flags: review?(past) → review+
Comment 11•6 years ago
|
||
(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•6 years ago
|
||
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6f0a242f6396 Upgrade Follow-on search add-on to 0.9.5. r=past
![]() |
||
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6f0a242f6396
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Is this something you're looking to uplift to 57?
Flags: needinfo?(standard8)
Assignee | ||
Comment 15•6 years ago
|
||
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
Flags: needinfo?(standard8)
Attachment #8915958 -
Flags: approval-mozilla-beta?
Comment on attachment 8915958 [details] Bug 1404126 - Upgrade Follow-on search add-on to 0.9.5. Must fix, beta57+
Attachment #8915958 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi Smaug, could you please verify this bug is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(bugs)
Comment 18•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/d055ef797ede
Reporter | ||
Comment 19•6 years ago
|
||
Hmm, this is trickier to reproduce since we handle child processes different after bug 1385249. I guess I should try beta.
Reporter | ||
Comment 20•6 years ago
|
||
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)
Flags: needinfo?(bugs)
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•