Initialization of the devtools menu slows down window opening

NEW
Unassigned

Status

()

Firefox
Developer Tools
P2
normal
8 months ago
3 days ago

People

(Reporter: florian, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
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
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!
(Reporter)

Comment 2

8 months ago
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?
Flags: needinfo?(enndeakin)
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

8 months ago
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.
Flags: needinfo?(enndeakin)
(Reporter)

Comment 5

8 months ago
(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?
Flags: needinfo?(bugs)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=834ab91a2fbb26b73451fbe5f07e5cd74782d6f6
(Reporter)

Comment 7

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

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

Comment 8

8 months ago
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.

Updated

8 months ago
Flags: needinfo?(bugs)

Comment 9

8 months ago
Is the code there setting lots of attributes?

Updated

8 months ago
Depends on: 1353811
Comment hidden (mozreview-request)
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...
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.
(Reporter)

Comment 13

8 months ago
(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).
(Reporter)

Comment 14

8 months ago
(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?
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%).
(Reporter)

Comment 16

8 months ago
(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

Updated

8 months ago
No longer blocks: 1348289
Whiteboard: [photon]
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.
Blocks: 1378418
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.