1349822 - [meta] updateCurrentBrowser can be slow

Status: NEW

(Firefox :: Tabbed Browser, enhancement, P5)

Description

[(no longer active)]

See this profile: https://perfht.ml/2n9r4hz

This slows down tab switching.  It seems like there is a lot of code running here, lots of xbl stuff too.  I haven't delved much into the details, it may be some Gecko slowness at fault, but filing in tabbed browser for now.

Comment 1

[:Gijs (he/him)]

This is almost entirely updateCurrentBrowser - https://perfht.ml/2n9r4hz . But it's not clear to me from that profile that there's a single culprit for that... the profile makes it look like just about everything is slow. Maybe Bill M or Mike C have ideas.

Comment 2

[(no longer active)]

It's probably worth profiling this in a more focused way.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It looks like:
  updateCurrentBrowser: 40ms
    some SDK crap (filter on loader.js): 12ms
    _recordTabAccess: 7ms

_recordTabAccess is some telemetry I added that we could remove now.

I'm not sure why there's some much SDK code in there. Maybe you have an add-on that's actually listening for this stuff, or maybe we just always do that? In any case, we should be able to fix this.

It's interesting that we spend about 16ms just compiling JS in this profile. Ehsan says he's pretty sure things have already warmed up. So we should look at whether we re-compile XBL every time we run it or something insane like that.

That's all the obvious low-hanging fruit I can see.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

The SDK stuff is where, I think, we should be investigating here.

I might not be reading this right (the SDK is mostly opaque to me), but it looks to me like if anything ever has imported Observer, we'll instantiate an Observer singleton:

http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/addon-sdk/source/lib/sdk/tabs/observer.js#113

and then run the initialize function for it, which adds an event listener for "select", which I _think_ maps to the TabSelect event by way of some indirection that I don't fully grok yet:

http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/addon-sdk/source/lib/sdk/tabs/observer.js#36-53

A quick sf through the tree shows that a bunch of other SDK modules import the observer module. I wonder if it's necessary to have at least one SDK add-on installed (like the Gecko Profiler!) to cause this event handler to be set.

I don't know who to ask about that. Gabor, do you know?

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) (Offsite until March 27) from comment #4)
> The SDK stuff is where, I think, we should be investigating here.
> 
> I might not be reading this right (the SDK is mostly opaque to me), but it
> looks to me like if anything ever has imported Observer, we'll instantiate
> an Observer singleton:
> 
> http://searchfox.org/mozilla-central/rev/
> c48398abd9f0f074c69f2223260939e30e8f99a8/addon-sdk/source/lib/sdk/tabs/
> observer.js#113

It's not you, it's opaque to all of us...

> 
> and then run the initialize function for it, which adds an event listener
> for "select", which I _think_ maps to the TabSelect event by way of some
> indirection that I don't fully grok yet:

I think so yes.

> 
> http://searchfox.org/mozilla-central/rev/
> c48398abd9f0f074c69f2223260939e30e8f99a8/addon-sdk/source/lib/sdk/tabs/
> observer.js#36-53
> 
> A quick sf through the tree shows that a bunch of other SDK modules import
> the observer module. I wonder if it's necessary to have at least one SDK
> add-on installed (like the Gecko Profiler!) to cause this event handler to
> be set.

This is only my best bet since I'm not that familiar with this code either.
I think when a chrome window is opened we get here: [1] and either register all the listeners
that is enumerated in EVENTS (with that confusing mapping for some reason...) on the
tabbrowser, OR we just save the tabbrowsers here and attach the listeners to
them only when someone actually wants to listen to some of those events. This is the more
likely version, in which case the first use for the TabSelect is the one you've spotted above.

Which observer module? There are many, so I just want to make sure that you're referring
to this one. It is necessary to have _something_ loading the tab module for this listener
to become active. Unfortunately I'm afraid devtools are still using this piece of beauty: [2]
so an SDK add-on is not a requirement. :(

> 
> I don't know who to ask about that. Gabor, do you know?

Hard to tell. Irakli should be the person to ask, but he is not very responsive. Maybe :Mossop or :ochameau.

[1]: http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/addon-sdk/source/lib/sdk/tabs/observer.js#58
[2]: http://searchfox.org/mozilla-central/search?q=tabs%2F&path=devtools

Comment 6

[:Gijs (he/him)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> (In reply to Mike Conley (:mconley) (Offsite until March 27) from comment #4)
> > I don't know who to ask about [sdk tabs and/or observer modules unecessarily causing slowness]. Gabor, do you know?
> 
> Hard to tell. Irakli should be the person to ask, but he is not very
> responsive. Maybe :Mossop or :ochameau.

:kmag improved SDK slowness and memory use a lot in bug 1309351, maybe he has ideas.

Comment 7

[Kris Maglione [:kmag]]

I think bug 1313767 might help with some of the event dispatching overhead.

I'm not sure what we can do about not registering the listener until something actually needs that event. Just thinking about trying to decipher that code gives me a headache...

Comment 8

[Kris Maglione [:kmag]]

Oh, jfc... https://people.mozilla.com/~kmaglione/images/e29499cb68e13e6a.png

The other big chunk of overhead is from the UI button code. I'm not touching that mess...

Let me explain roughly how it works:

It takes a bunch of events from a bunch of different places, pipes them through a bunch of filters, serializes them into objects, pipes them through a bunch more filters. Builds those objects into the current state of things. Runs an object diff on that state. Uses that diff to dispatch new events. A bunch of indecipherable magic happens. A "mailbox" or two is involved at some point. Eventually the UI gets updated to reflect that change.

Comment 9

[(no longer active)]

One ultimate side effect seems to be setBadge being called <http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/addon-sdk/source/lib/sdk/ui/button/view.js#216>.

Comment 10

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari (busy) from comment #9)
> One ultimate side effect seems to be setBadge being called
> <http://searchfox.org/mozilla-central/rev/
> 7419b368156a6efa24777b21b0e5706be89a9c2f/addon-sdk/source/lib/sdk/ui/button/
> view.js#216>.

Yeah, and it does look like that could be made significantly more efficient. The problem is that, from the profile, it looks like most of the overhead is from the stuff that happens in between the event and there. But I suppose that between fixing that and bug 1313767 we might be able to shave a few ms off of that call chain.

Comment 11

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #10)
> (In reply to :Ehsan Akhgari (busy) from comment #9)
> > One ultimate side effect seems to be setBadge being called
> > <http://searchfox.org/mozilla-central/rev/
> > 7419b368156a6efa24777b21b0e5706be89a9c2f/addon-sdk/source/lib/sdk/ui/button/
> > view.js#216>.
> 
> Yeah, and it does look like that could be made significantly more efficient.
> The problem is that, from the profile, it looks like most of the overhead is
> from the stuff that happens in between the event and there. But I suppose
> that between fixing that and bug 1313767 we might be able to shave a few ms
> off of that call chain.

OK, makes sense (at least to the extent I can claim to understand any of what this SDK code is doing!)  Let's at least get a dependency going on for now.

Comment 12

[(no longer active)]

What's the timeline of the add-on SDK code in m-c?

Comment 13

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari (busy) from comment #12)
> What's the timeline of the add-on SDK code in m-c?

That's a complicated question. We're planning as if it's going to be removed in 57, but actual removal may take some time. I intend to turn off most of the tests at that point, though.

But that's only part of the story. We'll only actually be able to remove some parts of it, since the devtools still use the loader, and a lot of the core and utility modules.

And of course it's always possible there will be some extension uses the SDK that we're going to wind up having to special case, and support in 57+ ... :( But I'm going to push pretty hard against that, if it comes up.

Comment 14

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #13)
> (In reply to :Ehsan Akhgari (busy) from comment #12)
> > What's the timeline of the add-on SDK code in m-c?
> 
> That's a complicated question. We're planning as if it's going to be removed
> in 57, but actual removal may take some time. I intend to turn off most of
> the tests at that point, though.
> 
> But that's only part of the story. We'll only actually be able to remove
> some parts of it, since the devtools still use the loader, and a lot of the
> core and utility modules.

I haven't filed every single one of UI jank issues that I have seen with the add-on SDK code, but anecdotally speaking, I have seen add-on SDK code show up in profiles maybe half of the times for small-ish jank like this.  :(

The problem is that I spend most of my dealing with second long badness these days, but it's really important to do what we can do get rid of this code (if at all possible) for QF...

Is there any way to be more aggressive?  Should we ask the devtools team *now* to start moving their code off of the add-on SDK?  Do you have any sense of how much work we are talking about there?

> And of course it's always possible there will be some extension uses the SDK
> that we're going to wind up having to special case, and support in 57+ ...
> :( But I'm going to push pretty hard against that, if it comes up.

I thought there will be no traditional extensions post 57?  I have been ignoring all sorts of add-on related perf issues due to that assumption all along.  Can you please clarify what types of extensions those will be?

Assuming we can't get rid of this code en masse, is there a smaller level of granularity that we can attack?

Comment 15

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari (busy) from comment #14)
> Is there any way to be more aggressive?  Should we ask the devtools team
> *now* to start moving their code off of the add-on SDK?  Do you have any
> sense of how much work we are talking about there?

As far as I know, the devtools don't use any of the SDK UI code, just the
low-level utility modules. Some of that code is still problematic, but between
bug 1313767 and bug 1314861, I don't think the devtools code will be a major
problem.

If they *do* use any of the UI code I'm not aware of then, yes, they should
start moving off of it now. I intend to remove most of that from the tree as
soon as possible.

> > And of course it's always possible there will be some extension uses the SDK
> > that we're going to wind up having to special case, and support in 57+ ...
> > :( But I'm going to push pretty hard against that, if it comes up.
> 
> I thought there will be no traditional extensions post 57?  I have been
> ignoring all sorts of add-on related perf issues due to that assumption all
> along.  Can you please clarify what types of extensions those will be?

System add-ons will still be allowed to be implemented as traditional add-ons,
and we have the ability to sign certain non-WebExtension add-ons with a
special cert to allow them to still load. But I believe the plan is to only
allow that for certain Mozilla add-ons.

> Assuming we can't get rid of this code en masse, is there a smaller level of
> granularity that we can attack?

My plan is to get rid of most of the UI and extension loading code as soon as
possible, but even if we don't get rid of it right away, it should not
actually be loaded.

Comment 16

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #15)
> (In reply to :Ehsan Akhgari (busy) from comment #14)
> > Is there any way to be more aggressive?  Should we ask the devtools team
> > *now* to start moving their code off of the add-on SDK?  Do you have any
> > sense of how much work we are talking about there?
> 
> As far as I know, the devtools don't use any of the SDK UI code, just the
> low-level utility modules. Some of that code is still problematic, but
> between
> bug 1313767 and bug 1314861, I don't think the devtools code will be a major
> problem.

Fantastic!  Are you planning on working on those bugs?

> If they *do* use any of the UI code I'm not aware of then, yes, they should
> start moving off of it now. I intend to remove most of that from the tree as
> soon as possible.

Great.  Is there a bug on file for that work?

> > > And of course it's always possible there will be some extension uses the SDK
> > > that we're going to wind up having to special case, and support in 57+ ...
> > > :( But I'm going to push pretty hard against that, if it comes up.
> > 
> > I thought there will be no traditional extensions post 57?  I have been
> > ignoring all sorts of add-on related perf issues due to that assumption all
> > along.  Can you please clarify what types of extensions those will be?
> 
> System add-ons will still be allowed to be implemented as traditional
> add-ons,
> and we have the ability to sign certain non-WebExtension add-ons with a
> special cert to allow them to still load. But I believe the plan is to only
> allow that for certain Mozilla add-ons.

OK, those are (presumably) under our control, so I'm not too worried about them.

> > Assuming we can't get rid of this code en masse, is there a smaller level of
> > granularity that we can attack?
> 
> My plan is to get rid of most of the UI and extension loading code as soon as
> possible, but even if we don't get rid of it right away, it should not
> actually be loaded.

If we don't remove the code, I suggest somehow ensuring that it cannot be loaded it possible, but reading this comment overall made me feel better about the situation since it sounds like you're completely on top of things.  Please let me know if you ever needed help moving something forward.  :-)

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to :Ehsan Akhgari (busy) from comment #16)
> (In reply to Kris Maglione [:kmag] from comment #15)
> > If they *do* use any of the UI code I'm not aware of then, yes, they should
> > start moving off of it now. I intend to remove most of that from the tree as
> > soon as possible.
> 
> Great.  Is there a bug on file for that work?

I am not sure which are the UI modules you intend to remove, but I filed bug 1350645 with a list of currently used SDK modules in DevTools.  Let's discuss more about the DevTools case there.

Comment 18

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari (busy) from comment #16)
> (In reply to Kris Maglione [:kmag] from comment #15)
> > As far as I know, the devtools don't use any of the SDK UI code, just the
> > low-level utility modules. Some of that code is still problematic, but
> > between bug 1313767 and bug 1314861, I don't think the devtools code will be
> > a major problem.
>
> Fantastic!  Are you planning on working on those bugs?

Yes, they both already have patches. I just posted updated patches for bug
1313767. I've been meaning to get back to bug 1314861, mainly for the sake of
e10s-multi, but have had too many other priorities. I suppose I'll go back to it
now, since I'm working on other SDK stuff anyway.

> > If they *do* use any of the UI code I'm not aware of then, yes, they should
> > start moving off of it now. I intend to remove most of that from the tree as
> > soon as possible.
>
> Great.  Is there a bug on file for that work?

No, I think about it every time I'm waiting for for the SDK test add-ons to
finish building at the end of a build, which has served the same tracking
purpose for me as a bug. But I just filed bug 1350646.

> If we don't remove the code, I suggest somehow ensuring that it cannot be
> loaded it possible, but reading this comment overall made me feel better
> about the situation since it sounds like you're completely on top of things.

We can probably start by removing them from the list of files that get packaged
in the moz.build. If we don't run into any trouble after a few weeks, we can
remove the files entirely.

> Please let me know if you ever needed help moving something forward.  :-)

I'll keep that in mind. Thanks

Comment 19

[Mike Conley (:mconley) (:⚙️)]

ni?ing myself to profile the hell out of this.

Comment 20

[Florian Quèze [:florian]]

Here is a profile on a very slow machine where updateCurrentBrowser alone takes 250+ms: https://perfht.ml/2oz1kh7

Comment 21

[Kris Maglione [:kmag]]

(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> Here is a profile on a very slow machine where updateCurrentBrowser alone
> takes 250+ms: https://perfht.ml/2oz1kh7

On the upside, at least 81ms of that is SDK gunk from the profiler add-on.