49 bytes, text/x-github-pull-request
|Details | Review|
59 bytes, text/x-review-board-request
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
It is very possible that some of that slowness moves elsewhere when the addon is disabled. I'll reprofile after disabling it.
Except that I don't know how to disable it. I thought it was part of Search Shield Study but apparently not.
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.
Yes, accessing the 'content' triggers about:blank creation, I think.
Assignee: nobody → standard8
Status: NEW → ASSIGNED
Priority: -- → P1
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.
[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.
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 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+
(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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/6f0a242f6396 Upgrade Follow-on search add-on to 0.9.5. r=past
Is this something you're looking to uplift to 57?
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
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!
Hmm, this is trickier to reproduce since we handle child processes different after bug 1385249. I guess I should try beta.
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)
You need to log in before you can comment on or make changes to this bug.