1500691 - Check if the picker and frame buttons did change before calling setToolboxButtons

Status: NEW

(DevTools :: Framework, defect, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Screen Shot 2018-10-20 at 11.17.46 AM.png

ToolboxController.js#setToolboxButtons is called when a tool is selected on startup and accounts for 45ms and is actually called 3 times on startup. We should check if the toolbox buttons visibility did change before calling setState.

console.log: "setToolboxButtons"
console.trace:
resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/components/ToolboxController.js 173 setToolboxButtons
resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js 1229 _buildButtons
resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js 515 open/<
console.log: "setToolboxButtons"
console.trace:
resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/components/ToolboxController.js 173 setToolboxButtons
resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js 2418 toolbarUpdate
resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js 2434 _updateFrames
resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js 2331 _listFrames
console.log: "setToolboxButtons"
console.trace:
resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/components/ToolboxController.js 173 setToolboxButtons
resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js 2715 _onToolSelected
resource://devtools/shared/base-loader.js -> resource://devtools/shared/event-emitter.js 178 emit
resource://devtools/shared/base-loader.js -> resource://devtools/shared/event-emitter.js 255 emit
resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js 1958 selectTool/<

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1500691.patch [1.0]

This looks like it will shave off 70ms which is pretty significant.

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1500691.patch [2.0]

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 9019247 [details] [diff] [review]
1500691.patch [2.0]

Review of attachment 9019247 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Gabriel! I do see a small difference in my profiles:
- with your patches: https://perfht.ml/2RcPdSc total 8ms in selectTool
- without your patches: https://perfht.ml/2RdjK2l total 18ms in selectTool

So roughly 10ms gain, less than what you measured but my computer is usually very bad to detect performance issues.

My main comment is that I think we only care about the visibility here? So we might be able to simplify the patch a bit.

::: devtools/client/framework/toolbox.js
@@ +1477,4 @@
>        this.frameButton.isChecked = selectedFrame.parentID != null;
> +      // Return false since isChecked will already cause a force update of the frame
> +      // button.
> +      return false;

`set isChecked()` blindly emits "updatechecked", maybe it should only emit it if the value has changed. Although that means this logic also would need to change to only return false if the value changed...

Probably better for a follow-up bug.

@@ +2458,1 @@
>      this.updateFrameButton();

It looks like this was already doing something similar to your patch? We should be able to know if the button was updated by reading the return value of `this.updateFrameButton();` now?

Performance optimizations are a bit scattered and it is getting hard to follow. I wonder if updateFrameButton and updatePickerButton should not simply call a debounced setToolboxButtons, if anything changed in the button. This way we can call them as many times as we want, and they don't have to return their status. What do you think?

@@ +2771,5 @@
> +    updateToolboxButtons |= this.updatePickerButton();
> +    updateToolboxButtons |= this.updateFrameButton();
> +
> +    if (updateToolboxButtons) {
> +      this.component.setToolboxButtons(this.toolbarButtons);

Two comments:
1/ Are the picker and frame buttons the only buttons that might change when we select a tool? Needs a comment to explain that this is why we are only setting toolbox buttons if something changed in one of those 2 buttons.
2/ I think setToolboxButtons only cares about the visibility of the buttons, the rest directly happens through DOM APIs. So you should only have to check if the visibility changed in both methods. I am not sure why the similar existing code from :birtles also checks "wasDisabled", maybe we should ask.

Comment 4

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 3 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.