1320149 - gcli takes between 20 to 40% of toolbox open time

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Alexandre Poirot [:ochameau]]

See number from bug 1320148, gcli takes about 20 to 40%, something around 1s on my laptop to load. And as it is part of the toolbox startup it slow the toolbox and tool startup.

I think the main issue is that it loads every of its dependencies on startup,
there is no or very limited lazy loading.

I don't know what are the best opportunities here:
 * do we need gcli just for the few buttons we have in the toolbox?! at the end we just call simple things ?
 * can we somewhat easily make gcli load its dependencies more lazily?
 * we could delay gcli load, but that may freeze the tool while user is surely going to interact with them

Comment 1

[Alexandre Poirot [:ochameau]]

Greg, I think you are working on toolbox toolbar button, may be you are already working on something for the gcli buttons??

Comment 2

[Joe Walker [:jwalker]]

Thanks for digging into this.

The original idea was that we would encourage people to add commands and then add the to the toolbox, but we've not followed that through, so just not using GCLI for this makes the most sense to me.

Comment 3

[Greg Tatum [:gregtatum]]

Yeah, I think removing any knowledge of the GCLI from the toolbox seems like a good idea here. I have a patch waiting for review on changing the Toolbar buttons to use HTML, which would probably be good to land first. I think this will have the added benefit of making the toolbar's implementation simpler.

Comment 4

[Greg Tatum [:gregtatum]]

Off the top of my head it seems to make sense to put the button definitions in:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/definitions.js

Then use the toolbox.js to convert the set of definitions into buttons.

Comment 5

[Alexandre Poirot [:ochameau]]

Great! Yes, having that in definitions makes a lot of sense.

Would you have cycles to look at that? Otherwise could you make that bug depends on the HTML refactoring so that I can watch your progress.

Comment 6

[Greg Tatum [:gregtatum]]

I'm getting ready to drop some of this current work for some P0 performance tooling work, so unfortunately I probably won't. I'll set this bug dependent on Bug 1245921.

Comment 7

[Alexandre Poirot [:ochameau]]

In an attempt to figure out how slow things are on linux32-debug I looked at gcli and toolbox load time:
Gcli load time:
  Min: 1.0s
  Average: 1.2s
  Max: 3.3s (browser_rules_completion-new-property_02.js)

Toolbox load time:
  Min: 5.4s
  Average: 6.5s
  Max: 11.7s (browser_rules_add-property-and-reselect.js)

It may be interesting to see why these two tests are so slow. May be previous tests still run something?

Comment 8

[Tim Nguyen :ntim]

I'm all for perf improvements, but the features exclusively available there should be ported to the toolbox:
- CSS coverage
- Different screenshot options
- security command (inspecting page CSP)
- Media query emulation
- Maybe calllog ? Although I doubt any Firefox/add-on dev uses it/knows about it.

A toolbox-only command palette could potentially be useful as well.

Comment 9

[Joe Walker [:jwalker]]

The developer toolbar will still exist, just the toolbox won't depend on it, so I don't think we should hold up this perf win on porting the commands to the toolbox.

Comment 10

[Alexandre Poirot [:ochameau]]

I'll try to craft a patch once bug 1245921 is landed.
We don't have to completely rip off gcli, we just need to prevent loading tons of modules.
But bug 1245921 is refactoring this very precise buttons code, so it is hard to say how to accommodate yet.

Comment 11

[Alexandre Poirot [:ochameau]]

Just a quick heads up, I still have this on my target, waiting for bug 1245921 to proceed.

But here is a profile on today's master, where I just opened the inspector:
https://new.cleopatra.io/public/e4b10f9c09d920dab6c0923adfec60a0457fcfbe/calltree/?jsOnly&search=gcli&thread=0
We can easily see the impact of gcli. Both in parent (>500ms) and in child (400ms) processes. (\o/ to the new cleopatra!)
It is even worse when you realize that in most cases, parent and child are waiting for each others to process a request before moving forward to the next thing to load!

Comment 12

[Alexandre Poirot [:ochameau]]

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

Comment 13

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=697551d8a944

Comment 14

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84c3833089eb

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1320149 - Prevent loading gcli when opening a toolbox.

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

Comment 16

[Alexandre Poirot [:ochameau]]

Mike, I chosed you as reviewer as you already looked at bug 1245921 and I'm patching the same code.
But feel free to forward or ask someone else to look at this patch.
For some buttons I completely bypass gcli and call the targeted code directly, for some others like rulers, paintflashing I'm still using gcli but lazily, so that the toolbox startup should be faster.
But now clicking on these buttons for the first time (the one still using gcli) could be significantly slower as it will load all gcli at that time.

Comment 17

[Alexandre Poirot [:ochameau]]

Some numbers from the profiler.

# with the patch:
  https://clptr.io/2jAxeUX
  Still a few things, I should lazy load paint-flashing module...
  
# without the patch:
  https://clptr.io/2jbSVhD
  500ms in the parent process
  400ms in the child process
  Significant part of the load timeline. All the blue in the timeline shows things related to gcli.

By the way, this patch is going to prevent showing any other gcli command buttons.
I imagine addons may register some new one? Is it the case? Is it a common addon usecase?

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

https://reviewboard.mozilla.org/r/104234/#review104966

Awesome patch, it really makes the toolbox startup feel snappier to me.

Just a few nits about the comments in startup.properties... I would r+ the patch but I guess doing it this way works better on ReviewBoard.

::: devtools/client/locales/en-US/startup.properties:277
(Diff revision 1)
> +# the Responsive mode.
> +# Keyboard shortcut will be shown inside brackets.
> +toolbox.buttons.responsive = Responsive Design Mode (%S)
> +
> +# LOCALIZATION NOTE (toolbox.buttons.paintflashing):
> +# This is the tooltip of the  button in the toolbox toolbar

This is the tooltip of the paintflashing button in the toolbox toolbar that toggles paintflashing.

::: devtools/client/locales/en-US/startup.properties:281
(Diff revision 1)
> +# LOCALIZATION NOTE (toolbox.buttons.paintflashing):
> +# This is the tooltip of the  button in the toolbox toolbar
> +toolbox.buttons.paintflashing = Toggle paint flashing
> +
> +# LOCALIZATION NOTE (toolbox.buttons.scratchpad):
> +# This is the tooltip of the button in the toolbox toolbar that opens

opens the scratchpad window.

::: devtools/client/locales/en-US/startup.properties:286
(Diff revision 1)
> +# This is the tooltip of the button in the toolbox toolbar that opens
> +# scratchpad window
> +toolbox.buttons.scratchpad = Scratchpad
> +
> +# LOCALIZATION NOTE (toolbox.buttons.screenshot):
> +# This is the tooltip of the button in the toolbox toolbar that allows to take

allows you to take

::: devtools/client/locales/en-US/startup.properties:291
(Diff revision 1)
> +# This is the tooltip of the button in the toolbox toolbar that allows to take
> +# a screenshot of the entire page
> +toolbox.buttons.screenshot = Take a screenshot of the entire page
> +
> +# LOCALIZATION NOTE (toolbox.buttons.rulers):
> +# This is the tooltip of the button in the toolbox toolbar thats toggles the

s/thats/that/

::: devtools/client/locales/en-US/startup.properties:296
(Diff revision 1)
> +# This is the tooltip of the button in the toolbox toolbar thats toggles the
> +# rulers in the page
> +toolbox.buttons.rulers = Toggle rulers for the page
> +
> +# LOCALIZATION NOTE (toolbox.buttons.measure):
> +# This is the tooltip of the button in the toolbox toolbar thats toggles the

s/thats/that/

Comment 19

[Alexandre Poirot [:ochameau]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104234/diff/1-2/

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

I would feel better with your positive feedback on such patch!
I'm especially worried about a possible addon usage of the previous code that won't work anymore.

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

https://reviewboard.mozilla.org/r/104234/#review105144

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104234/diff/2-3/

Comment 23

[Joe Walker [:jwalker]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

https://reviewboard.mozilla.org/r/104234/#review106258

::: devtools/client/framework/toolbox.js:579
(Diff revision 3)
>      const button = {
>        id,
>        className,
>        description,
> -      onClick,
> +      onClick(event) {
> +        if (onClick) {

`if (typeof onClick === 'function') ...`?

::: devtools/client/framework/toolbox.js:583
(Diff revision 3)
> -      onClick,
> +      onClick(event) {
> +        if (onClick) {
> +          onClick(event, toolbox);
> +        }
> +      },
> +      isTargetSupported: options.isTargetSupported,

Add isTargetSupported to the doc comment and the const so we can just say `isTargetSupported,` here?

::: devtools/client/framework/toolbox.js:585
(Diff revision 3)
> +          onClick(event, toolbox);
> +        }
> +      },
> +      isTargetSupported: options.isTargetSupported,
>        get isChecked() {
> +        if (typeof options.isChecked == "function") {

Also isChecked

::: devtools/client/framework/toolbox.js:609
(Diff revision 3)
> +      };
> +      setup(this, onChange);
> +      // Save a reference to the cleanup method that will unregister the onChange
> +      // callback. Immediately bind the function argument so that we don't have to
> +      // also save a reference to them.
> +      button.teardown = options.teardown.bind(options, this, onChange);

Same comment for `teardown`

::: devtools/client/framework/toolbox.js:1067
(Diff revision 3)
> -        spec, this.target, this.doc, this._requisition, this._createButtonState);
> -      this.toolbarButtons = [...this.toolbarButtons, ...commandButtons];
> -    }
>  
> -    // Mutate the objects here with their visibility.
> -    this.toolbarButtons.forEach(command => {
> +    // Set isVisible after adding button to toolbarButtons as commandIsVisible
> +    // expects to find the button is the array.

But as far as I can see there's no good reason for this, we could just change the definition of `_commandIsVisible` to take a command rather than an id?

Comment 24

[Joe Walker [:jwalker]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

Not sure if you can f+ from reviewboard.

Comment 25

[Alexandre Poirot [:ochameau]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104234/diff/3-4/

Comment 26

[Alexandre Poirot [:ochameau]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104234/diff/4-5/

Comment 27

[Alexandre Poirot [:ochameau]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

Mike, I had to change a few things.
Revision 2 on reviewboard is addressing your comments,
3 is a just a rebase,
4 and 5 is adressing joe's comments. In this revision I introduced "autoToggle" in order to prevent loading gcli at all. Without that I had to pull paintflashing/rulers/measure commands to know the checked state of each of the buttons. But that pulls a bunch of deps again (you can see them in comment 17 first cleopatra link)

Comment 28

[Alexandre Poirot [:ochameau]]

Joe, I'm not sure you saw comment 20. The feedback request wasn't so much about the code, but more about the consequences of such patch. We are no longer going to show any gcli command button in the toolbox. That's fair for devtools one as I converted them, but I'm concerned about addons.

It looks like there is at least firebug, adding gcli commands:
https://github.com/firebug/firebug/blob/313e5bece064c0fb0ab7db123a38ba3b90dfc922/extension/modules/gcli.js#L146-L182

Should we move forward with this? Is it fine breaking potential usage of gcli buttons of addons?

Comment 29

[Alexandre Poirot [:ochameau]]

Honza, do we still support Firebug in Firefox 53? Is it ok if we start breaking firebug gcli buttons in Firefox 53?

Comment 30

[Joe Walker [:jwalker]]

With e10s disabling FB soon, and the fact that I'm not sure if those FB command still work anyway, and that this would only affect people that had customized their toolbox with those commands, and that in the wider context, I'm not aware of anyone else adding new commands, this is OK by me.

Comment 31

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #29)
> Honza, do we still support Firebug in Firefox 53? Is it ok if we start
> breaking firebug gcli buttons in Firefox 53?
It's ok. GCLI buttons are disabled in the latest Firebug version.

Honza

Comment 32

[Alexandre Poirot [:ochameau]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104234/diff/5-6/

Comment 33

[Alexandre Poirot [:ochameau]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104234/diff/6-7/

Comment 34

[Alexandre Poirot [:ochameau]]

Thanks for the green light Jan and Joe!

Mike, sorry, I piled up yet another tweaks.
Revision 6 is removing the now useless "toolbarSpec" pref, it allowed me to prevent a regression around screenshot to clipboard pref...
Revision 7 is another rebase to be on top of latest m-c.

Comment 35

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

https://reviewboard.mozilla.org/r/104232/#review105142

Comment 36

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

https://reviewboard.mozilla.org/r/104234/#review109060

Comment 37

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Sorry the review took so long... I did it a few days ago but I guess I forgot to click publish.

Comment 38

[Alexandre Poirot [:ochameau]]

Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104234/diff/7-8/

Comment 39

[Alexandre Poirot [:ochameau]]

New try after rebase:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02563159e9f0f919e74faff11f95bba7a8ccaf47

And talos, but still computing...
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=82cec5cbf5f100e07455846b3f649a500e7a3f30&newProject=try&newRevision=02563159e9f0f919e74faff11f95bba7a8ccaf47&framework=1&showOnlyImportant=0

Comment 40

[Alexandre Poirot [:ochameau]]

5-6% win on talos for windows and mac (linux doesn't want to run...)

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6895ed1367c
Prevent loading gcli when opening a toolbox. r=jwalker,miker

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f6895ed1367c

Comment 43

[Joel Maher ( :jmaher ) (UTC -8)]

kudos for this patch, perfherder says damp improved:
== Change summary for alert #4961 (as of January 30 2017 19:14 UTC) ==

Improvements:

  9%  damp summary linux64 pgo         286.04 -> 260.11
  9%  damp summary linux64 opt         322.96 -> 295.5
  8%  damp summary linux64 pgo         268.35 -> 245.78
  8%  damp summary windows8-64 opt     341.66 -> 314.76
  7%  damp summary windows7-32 opt     359.13 -> 335
  7%  damp summary linux64 opt         336.06 -> 314.12
  6%  damp summary osx-10-10 opt       335.16 -> 315.1

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4961