1353548 - Initialization of the devtools menu slows down window opening

Status: NEW

(DevTools :: General, defect, P3)

Description

[Florian Quèze [:florian]]

When profiling window opening, the observer from devtools-browser.js calling _registerBrowserWindow (and more specifically addMenus from browser-menus.js) is where most of the time spent in _delayedStartup goes.

See this profile: https://perf-html.io/public/65788883411d02c96c3ee5795a10df0010b1667a/calltree/?jsOnly&range=13.6152_23.1418&search=_delayedStartup&thread=0

Comment 1

[Alexandre Poirot [:ochameau]]

It looks like most of the time is spent creating xul elements.

This is surprising to see such simple functions like createKey and createMenu to take that much time.
For createKey, most of the time seems to be spent in DOMElement::setAttribute.
Itself ends up going through XULElement::parseAttribute, which spend most of its time doing:
  PLDHashTable::Add(void const*, mozilla::fallible_t const&)

I can't believe this code is slow because of adding attribute values in a hash table?!

Also I see cross compartment wrappers. I'm wondering if that can be faster if the script injecting the elements lives in the same compartment than the elements. But I hope wrappers aren't that slow!

Comment 2

[Florian Quèze [:florian]]

It's indeed surprising that ParseAttribute / PLDHashTable::Add takes half the time here. Neil, do you know why this is so slow?

Cross compartement wrappers add a bit of overhead, yes... but I don't think that's significant here compared to the DOM manipulations, which are known to be slow. Is there a way to completely avoid doing this when opening a window? Eg. could this be inserted in the browser.xul window using a xul overlay (which I think would be cached after the first time browser.xul is loaded) rather than using DOM manipulations in JS? Or would there be a way to do this stuff when the tools menu is opened for the first time, rather than when the window loads?

Comment 3

[Alexandre Poirot [:ochameau]]

I would prefer avoiding overlay. I wouldn't be surprise if we get rid of them soon.
Also, I'm not sure we are still going to be able to do that when devtools are going to be an addon (we are working on this).
On menu opening sounds like a good option.

Comment 4

[Neil Deakin]

I'm not sure why the hashtable adding is slow; maybe this is bug 1351303?

On non-Mac you could instead initialize during the popupshowing event for the menu, assuming it is slow on all platforms. You won't be able to do that on Mac or the keyboard shortcuts won't work properly.

Comment 5

[Florian Quèze [:florian]]

(In reply to Neil Deakin from comment #4)
> I'm not sure why the hashtable adding is slow; maybe this is bug 1351303?

Seems very likely. In this case, do we need the NS_Atomize call at https://hg.mozilla.org/mozilla-central/annotate/1825773142a0/dom/base/nsAttrValue.cpp#l1246 changed to NS_AtomizeMainThread?

Comment 6

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=834ab91a2fbb26b73451fbe5f07e5cd74782d6f6

Comment 7

[Florian Quèze [:florian]]

(In reply to Alexandre Poirot [:ochameau] from comment #6)

Have you considered using {once: true} in the addEventListener call?

Comment 8

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

Hashtables are slow in general when we're talking about hot code paths. (Same with malloc/free.)
And NS_Atomize in particular is slow, because it is threadsafe.

If NS_AtomizeMainThread usage is added, could you please verify the cache NS_AtomizeMainThread has catches enough cases, like create another performance profile with that to verify hashtable usage goes down.
If so, the change to use NS_AtomizeMainThread feels reasonable.

Comment 9

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

Is the code there setting lots of attributes?

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1353548 - Create devtools menu entries on menu showing.

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

Comment 11

[Alexandre Poirot [:ochameau]]

103 calls to setAttribute.

But I get a completely different profile here:
  https://perfht.ml/2nGTr5b
This is with the patch, adding the xul menuitems and keys only once the menu is opened.
In this profile createKey and createMenuitem (both functions doing a bunch of setAttribute), don't appear to be slow...

Comment 12

[Alexandre Poirot [:ochameau]]

Note that this patch is just to see the difference. This injects the key shortcuts only after user opened the tools menu.
And there is broken tests...

Now I'm wondering if the original profile was correct, I suspect the cost of devtools to be related to something else than xul setAttribute.
How are you able to record firefox startup with profiler addon?
Mine seems to only register *after* delayed startup.

Comment 13

[Florian Quèze [:florian]]

(In reply to Alexandre Poirot [:ochameau] from comment #12)

> How are you able to record firefox startup with profiler addon?
> Mine seems to only register *after* delayed startup.

The profile I attached was not of startup, but of opening new browser windows (ie. pressing cmd+n).

That said, if you want to profile startup, you can start your build with the MOZ_PROFILER_STARTUP environment variable (but I'm not confident yet in the results I see when doing that).

Comment 14

[Florian Quèze [:florian]]

(In reply to Alexandre Poirot [:ochameau] from comment #11)

> In this profile createKey and createMenuitem (both functions doing a bunch
> of setAttribute), don't appear to be slow...

I'm assuming you are profiling on a fast machine. This is also what I was doing, so I actually opened 7 windows in my profile so that the time spent while opening windows would become significant.

From looking at your profile, it seems you now initialize the devtools l10n helpers the first time the tools menu is opened, whereas without your patch it was likely initialized during startup, so not visible on the profiles of opening new windows.

This l10n helper seems expensive; could we delay its initialization until the devtools are actually used, and use the normal Services.strings.createBundle instead while building the menu?

Comment 15

[Alexandre Poirot [:ochameau]]

That's on a XPS 13, within a not so fast virtual machine on Ubuntu.

Here is a new profile against current master (without any patch)
  https://perfht.ml/2oMkM84

XULElement::setAttribute doesn't appear in this profile either (only 12ms), could it be specific to only one platform?
Also the inner cost of devtools is much more diffuse.
Still, devtools menu/key shortcut setup is around 60ms out of 190ms for delayed-startup (30%).

Comment 16

[Florian Quèze [:florian]]

(In reply to Alexandre Poirot [:ochameau] from comment #15)
> That's on a XPS 13, within a not so fast virtual machine on Ubuntu.
> 
> Here is a new profile against current master (without any patch)
>   https://perfht.ml/2oMkM84
> 
> XULElement::setAttribute doesn't appear in this profile either (only 12ms),
> could it be specific to only one platform?

Maybe. It's also possible that some performance improvement patches landed since I capture the profile from comment 0 (it was capture more than a week ago). Here is a profile similar to the one on comment 0, but on a build from yesterday: https://perfht.ml/2oMuHKO

Comment 17

[Alexandre Poirot [:ochameau]]

We are most likely going to refactor this code soon in order to move devtools as an addon.
We want all, or at least a subset of common devtools key shortcut to still be defined even if devtools are not installed.
I would like to defer tweaking this potential performance while doing that.

It is not obvious. We have to create the <key> at startup. We can't wait for menu opening to register the key shortcut.
And the key shortcuts also depends on l10n. So even if we delay <menuitem> creation, I'm expecting to still see a significant time being spent in <key> creation. And given that it wouldn't be a simple refactoring, I would prefer to do the "devtools as an addon" refactoring at the same time.

I'll start opening bugs and reference this one.

Comment 18

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

(In reply to Alexandre Poirot [:ochameau] from comment #17)
> We are most likely going to refactor this code soon in order to move
> devtools as an addon.

Is this still the plan?

Comment 19

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

Also, have we considered making this code not block painting the window by making it lazier? Perhaps only injecting the <key> nodes at start-up, and only injecting the menu items if the Tools menu is opened? Or, at the very least, doing this work inside an idle callback?

Comment 20

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

Hey sole, do you know if the plan is still to move DevTools out into some kind of add-on?

Comment 21

[Alexandre Poirot [:ochameau]]

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #20)
> Hey sole, do you know if the plan is still to move DevTools out into some
> kind of add-on?

No, this is no longer the plan.

I'm not sure this bug is still relevant since bug 1359855 landed?
That bug delayed the whole devtools setup to be trigerred only after the first used interaction related to devtools.
So it would be surprising if anything still runs.

(Note that if you already used devtools once in your current session, you may see devtools code running as before bug 1359855)

Comment 22

[Alexandre Poirot [:ochameau]]

Only this module should be loaded until the first real devtools interaction (key shortcut, command line argument, ...)
  https://searchfox.org/mozilla-central/source/devtools/startup/devtools-startup.js

For exemple, regarding the Web Developer menu, we only register a popup listener and populate the menu when it is opened:
  https://searchfox.org/mozilla-central/source/devtools/startup/devtools-startup.js#452-461

I think we could mark this bug as duplicate unless something regressed, or something else should be fixed?

Comment 23

[:Gijs (he/him)]

(In reply to Alexandre Poirot [:ochameau] from comment #22)
> Only this module should be loaded until the first real devtools interaction
> (key shortcut, command line argument, ...)
>  
> https://searchfox.org/mozilla-central/source/devtools/startup/devtools-
> startup.js
> 
> For exemple, regarding the Web Developer menu, we only register a popup
> listener and populate the menu when it is opened:
>  
> https://searchfox.org/mozilla-central/source/devtools/startup/devtools-
> startup.js#452-461
> 
> I think we could mark this bug as duplicate unless something regressed, or
> something else should be fixed?

It does look like the situation is a lot better on nightly than originally reported, but devtools is still about 10% of the delayedStartup time in a quick check on my regular current macOS nightly.

It looks like most of what it does is the shortcut creation - but all the shortcuts just point to the same handler. Perhaps we can hardcode them (if we're no longer planning to split off the add-on), or we can create a fragment that we then clone instead of creating things per-window, or we can even just create a single key handler that exits early for any events that don't have the right modifier state, and then does the same thing it does now (forwarding somewhere else) ?

It also looks like the lazy web developer menu listener addition could just be inlined to call a public method on this devtools-startup xpcom component that we have anyway. Maybe we can do these 2 things in two separate follow-ups or something?