Closed Bug 1320149 Opened 8 years ago Closed 7 years ago

gcli takes between 20 to 40% of toolbox open time

Categories

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

Product:
DevTools Graveyard
Component:
Graphic Commandline and Toolbar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1320149 - Prevent loading gcli when opening a toolbox.
59 bytes, text/x-review-board-request
miker
: review+
Details 
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
Greg, I think you are working on toolbox toolbar button, may be you are already working on something for the gcli buttons??
Flags: needinfo?(gtatum)
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.
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.
Flags: needinfo?(gtatum)
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.
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.
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.
Depends on: 1245921
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?
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.
Blocks: 1320790
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.
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.
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!
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.
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 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/
Attachment #8826237 - Flags: review?(mratcliffe)
Assignee: nobody → poirot.alex
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.
Attachment #8826237 - Flags: feedback?(jwalker)
Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

https://reviewboard.mozilla.org/r/104234/#review105144
Attachment #8826237 - Flags: review?(mratcliffe) → review+
Attachment #8826237 - Flags: feedback?(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?
Attachment #8826237 - Flags: review+
Comment on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

Not sure if you can f+ from reviewboard.
Attachment #8826237 - Flags: feedback?(jwalker) → feedback+
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)
Attachment #8826237 - Flags: review+ → review?(mratcliffe)
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?
Flags: needinfo?(jwalker)
Honza, do we still support Firebug in Firefox 53? Is it ok if we start breaking firebug gcli buttons in Firefox 53?
Flags: needinfo?(odvarko)
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.
Flags: needinfo?(jwalker)
(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
Flags: needinfo?(odvarko)
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 on attachment 8826237 [details]
Bug 1320149 - Prevent loading gcli when opening a toolbox.

https://reviewboard.mozilla.org/r/104234/#review109060
Attachment #8826237 - Flags: review?(mratcliffe) → review+
Sorry the review took so long... I did it a few days ago but I guess I forgot to click publish.
5-6% win on talos for windows and mac (linux doesn't want to run...)
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f6895ed1367c
Prevent loading gcli when opening a toolbox. r=jwalker,miker
https://hg.mozilla.org/mozilla-central/rev/f6895ed1367c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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
Depends on: 1346647
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: