Closed Bug 1339483 Opened 3 years ago Closed 3 years ago

Add-on SDK can slow down opening a new tab

Categories

(Add-on SDK Graveyard :: General, defect)

defect
Not set

Tracking

(firefox52- fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 verified)

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 - fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- verified

People

(Reporter: ehsan, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

See this profile: <https://perfht.ml/2lga1ey>.  It seems that we are doing some generic work in the add-on SDK in response to a new window being created which can cause the UI to jank.  In this profile topicListener <http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/addon-sdk/source/lib/sdk/content/tab-events.js#22> is taking 116ms.
CCing Kris since I know he's spent time trying to reduce the SDK overhead recently.
I suspect some of this might be fixed by the changes in bug 1314861. I lazified a bunch of the stuff that happened in remote/child, partly to reduce loading things before they were needed.

But it also looks like there's still a lot to do... Every time a {chrome,content}-document-{global-created,interactive,loaded} observer fires, we apparently iterate over a list of every ContentFrameMessageManager in the current process to see if the window belongs to any of them. (We apparently do this even if the window isn't a top-level window, even though non-top-level windows will never match).

The check for each frame compares the `content` property of a the SDK's Frame wrapper object against the window. That `content` property is a getter, which does a WeakMap lookup to get the object's private variables namespace, and then gets the ContentFrameMessageManager from that, and then the message manager's `content` object via XPConnect (via cross-compartment proxy).

To say nothing of all of the things that happen in the Frame object's constructor.

So, basically, all of this is done just about as inefficiently as possible, and without any attempt to avoid doing it in the cases where we can easily determine that it's not necessary.
Depends on: 1314861
Comment on attachment 8837313 [details]
Bug 1339483: Fix tab frame lookup performance issues.

https://reviewboard.mozilla.org/r/112472/#review113874
Attachment #8837313 - Flags: review?(dtownsend) → review+
Assignee: nobody → kmaglione+bmo
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4899a861d286
Fix tab frame lookup performance issues. r=mossop
Thanks for the super fast fix, much appreciated!
Let's get this on the radar for uplift too.
https://hg.mozilla.org/mozilla-central/rev/4899a861d286
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8837313 [details]
Bug 1339483: Fix tab frame lookup performance issues.

Approval Request Comment
[Feature/Bug causing the regression]: N/A

[User impact if declined]: This bug may cause serious performance issues, particular for users with many tabs, or multiple SDK add-ons.

[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: Reporter should verify or provide steps to reproduce.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: Low-risk.
[Why is the change risky/not risky?]: The changes are trivial optimizations which should result in the same behavior but with less overhead.

[String changes made/needed]: None.
Attachment #8837313 - Flags: approval-mozilla-release?
Attachment #8837313 - Flags: approval-mozilla-beta?
Attachment #8837313 - Flags: approval-mozilla-aurora?
Hi :Eshan,
could you help verify if this issue was fixed on a latest Nightly build? Thanks!
Flags: needinfo?(ehsan)
Comment on attachment 8837313 [details]
Bug 1339483: Fix tab frame lookup performance issues.

I don't think this meets the bar for a dot release.
Attachment #8837313 - Flags: approval-mozilla-release? → approval-mozilla-release-
(In reply to Gerry Chang [:gchang] from comment #10)
> Hi :Eshan,

It's Ehsan.  :-)

> could you help verify if this issue was fixed on a latest Nightly build?
> Thanks!

I tried opening around 10 tabs or so one after each other.  Now topicListener() only takes ~22ms <https://perf-html.io/public/55385c23113278e8db31a2fee58338fdbf04bfd1/calltree/?search=topicListener&thread=1>, so I'd say this is fixed.  Thanks again, Kris!
Flags: needinfo?(ehsan)
(In reply to Julien Cristau [:jcristau] from comment #11)
> I don't think this meets the bar for a dot release.

I agree, but I think it would be worth a ride-along if we wind up doing a dot release anyway.

(In reply to :Ehsan Akhgari from comment #12)
> I tried opening around 10 tabs or so one after each other.  Now
> topicListener() only takes ~22ms
> <https://perf-html.io/public/55385c23113278e8db31a2fee58338fdbf04bfd1/
> calltree/?search=topicListener&thread=1>, so I'd say this is fixed.  Thanks
> again, Kris!

Great. Thanks for testing.
Comment on attachment 8837313 [details]
Bug 1339483: Fix tab frame lookup performance issues.

Fix tab frame lookup performance issues and was verified. Aurora53+.
Attachment #8837313 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8837313 [details]
Bug 1339483: Fix tab frame lookup performance issues.

fix perf issue with add-on sdk, beta52+
Attachment #8837313 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.