Improve the Inspector load speed

RESOLVED FIXED in Firefox 64

Status

P3
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: gl, Assigned: gl)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 64
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(47 attachments, 19 obsolete attachments)

5.65 KB, patch
pbro
: review+
Details | Diff | Splinter Review
2.96 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.67 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.95 KB, patch
pbro
: review+
Details | Diff | Splinter Review
2.86 KB, patch
pbro
: review+
Details | Diff | Splinter Review
3.57 KB, patch
pbro
: review+
Details | Diff | Splinter Review
2.63 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.46 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.99 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.39 KB, patch
pbro
: review+
Details | Diff | Splinter Review
5.71 KB, patch
pbro
: review+
Details | Diff | Splinter Review
6.14 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.01 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.43 KB, patch
pbro
: review+
Details | Diff | Splinter Review
2.70 KB, patch
pbro
: review+
Details | Diff | Splinter Review
5.77 KB, patch
pbro
: review+
Details | Diff | Splinter Review
4.37 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.54 KB, patch
pbro
: review+
Details | Diff | Splinter Review
970.38 KB, image/png
Details
2.84 KB, patch
Honza
: review+
Details | Diff | Splinter Review
1.64 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
1.45 KB, patch
yzen
: review+
Details | Diff | Splinter Review
4.27 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
1.87 KB, patch
Honza
: review+
Details | Diff | Splinter Review
11.17 KB, patch
Details | Diff | Splinter Review
5.80 KB, patch
Honza
: review+
Details | Diff | Splinter Review
1.52 KB, patch
pbro
: review+
Details | Diff | Splinter Review
2.06 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
13.63 KB, patch
pbro
: review+
Details | Diff | Splinter Review
3.11 KB, patch
pbro
: review+
Details | Diff | Splinter Review
11.60 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
15.21 KB, patch
pbro
: review+
Details | Diff | Splinter Review
15.96 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.75 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
1.83 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.26 KB, patch
pbro
: review+
Details | Diff | Splinter Review
11.23 KB, patch
birtles
: review+
Details | Diff | Splinter Review
4.18 KB, patch
Details | Diff | Splinter Review
1.23 KB, patch
pbro
: review+
Details | Diff | Splinter Review
3.71 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.37 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
3.24 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
1.38 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
1.26 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
3.09 KB, patch
pbro
: review+
Details | Diff | Splinter Review
8.62 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.09 KB, patch
pbro
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

6 months ago
Keywords: leave-open
(Assignee)

Updated

6 months ago
Attachment #9012009 - Attachment description: art 1: Lazy load the HTMLEditor in the markup view. [1.0] → Part 1: Lazy load the HTMLEditor in the markup view. [1.0]
(Assignee)

Updated

6 months ago
Summary: Lazy load modules in the markup view → Lazy load modules in the inspector
(Assignee)

Comment 13

6 months ago
Attachment #9012033 - Flags: review?(pbrosset)
(Assignee)

Comment 16

6 months ago
Attachment #9012031 - Attachment is obsolete: true
Attachment #9012031 - Flags: review?(pbrosset)
Attachment #9012039 - Flags: review?(pbrosset)
Blocks: 1378418
Comment on attachment 9012009 [details] [diff] [review]
Part 1: Lazy load the HTMLEditor in the markup view. [1.0]

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

I don't feel for or against this change. It seems like it shouldn't have any effect performance-wise. Before the change, we used a normal require, but only when needed. And after the change, we use a lazyRequire.
Is this for consistency reasons?

::: devtools/client/inspector/markup/views/html-editor.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const Services = require("Services");

This change seems unrelated and unneeded. I would advise against changing this because it tends to polute the HG history for not much reason.
Attachment #9012009 - Flags: review?(pbrosset) → review+
Attachment #9012011 - Flags: review?(pbrosset) → review+
Comment on attachment 9012013 [details] [diff] [review]
Part 3: Lazy load the SlottedNodeContainer in the markup view.

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

::: devtools/client/inspector/markup/views/slotted-node-editor.js
@@ +5,5 @@
>  "use strict";
>  
>  const {LocalizationHelper} = require("devtools/shared/l10n");
>  const INSPECTOR_L10N =
> +  new LocalizationHelper("devtools/client/locales/inspector.properties");

This change is unrelated and will pollute the HG history unnecessarily. I advise reverting this change.
If there are many of these little formatting changes you want to make, better make them all in one initial commit, with a commit message that explains what was changed.
Attachment #9012013 - Flags: review?(pbrosset) → review+
Comment on attachment 9012015 [details] [diff] [review]
Part 4: Lazy load the modules in the TextEditor. [1.0]

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

::: devtools/client/inspector/markup/views/text-editor.js
@@ +12,3 @@
>  
>  const INSPECTOR_L10N =
> +  new LocalizationHelper("devtools/client/locales/inspector.properties");

Same comment as before about unrelated formatting changes.
Attachment #9012015 - Flags: review?(pbrosset) → review+
Attachment #9012017 - Flags: review?(pbrosset) → review+
Attachment #9012019 - Flags: review?(pbrosset) → review+
Comment on attachment 9012029 [details] [diff] [review]
Part 7: Lazy load modules in the shared inspector utils. [1.0]

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

::: devtools/client/inspector/shared/utils.js
@@ +9,1 @@
>  const promise = require("promise");

I guess this one could also be lazily required, right?
Attachment #9012029 - Flags: review?(pbrosset) → review+
Attachment #9012030 - Flags: review?(pbrosset) → review+
Comment on attachment 9012033 [details] [diff] [review]
Part 10: Lazy initialize the event details tooltip. [1.0]

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

::: devtools/client/inspector/markup/markup.js
@@ +190,1 @@
>      this.imagePreviewTooltip = new HTMLTooltip(this.toolbox.doc, {

Why not also use this commit to lazy load the image preview tooltip?
It seems arbitrary to me that we would lazy load one tooltip type, but not the other one.
Unless there's a technical limitation that prevents us from doing it on the image tooltip.
Attachment #9012033 - Flags: review?(pbrosset)
Attachment #9012035 - Flags: review?(pbrosset) → review+
Attachment #9012039 - Flags: review?(pbrosset) → review+
Attachment #9012040 - Flags: review?(pbrosset) → review+
Attachment #9012041 - Flags: review?(pbrosset) → review+
Here's a DAMP compare URL for the whole patch series: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=e61a896c934ac0fc60b69e7cc555494c2a57fb9f&newProject=try&newRevision=64c9d44142565db9254e31caad1b48253acaf300&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

So inspector open and reload on simple documents improved a lot.
These tests are good at showing improvements (or regressions) in the tool UI itself, so those are really good, and trusted, improvements.
Now, the other open and reload tests did not improve. This is most probably because they use more complex documents that require much more processing (especially server side), and the time needed for that dwarfs the improvements you've made.
(Assignee)

Comment 28

6 months ago
Attachment #9012148 - Attachment is obsolete: true
Attachment #9012148 - Flags: review?(pbrosset)
Attachment #9012150 - Flags: review?(pbrosset)
(Assignee)

Comment 29

6 months ago
We had to initialize the image tooltip differently because it needs to call _enableImagePreviewTooltip() which calls startTogglingOnHover() which allows the image tooltip to toggle on hover.
Attachment #9012033 - Attachment is obsolete: true
Attachment #9012154 - Flags: review?(pbrosset)
Comment on attachment 9012154 [details] [diff] [review]
Part 10: Lazy initialize the event details and image tooltip in the markup view. [2.0]

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

::: devtools/client/inspector/markup/markup.js
@@ +122,5 @@
> +  if (flags.testing) {
> +    // In tests, we start listening immediately to avoid having to simulate a mousemove.
> +    this._initTooltips();
> +  } else {
> +    this._elt.addEventListener("mousemose", () => {

typo: mousemose is not a valid event name :)
Attachment #9012154 - Flags: review?(pbrosset) → review+
Attachment #9012147 - Flags: review?(pbrosset) → review+
Attachment #9012150 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 35

6 months ago
Very minor 0.1ms savings
Attachment #9012455 - Flags: review?(pbrosset)
(Assignee)

Comment 37

6 months ago
This removes around 2.2ms. See profile screenshot to be attached.
Attachment #9012461 - Flags: review?(pbrosset)
(Assignee)

Comment 38

6 months ago
Posted image Profile of Part 22
I was curious to see how this patch series would perform on my low-ish end MacBook, so I ran a few tests.

1. on about:blank
  a. without the patches
     cold open of the inspector: 400ms
     re-open of the inspector: 290ms
  b. with the patches
     cold open: 380ms --> 5% win
     re-open: 280ms   --> 3.4% win

These tests show some interesting perf wins. On this simple page, there are almost no DOM nodes and no stylesheets, so the server-side processing is down to a minimum. So these tests do show UI improvements more easily.
I would have expected a much bigger difference, but it's a start.
As discussed yesterday, the URL in comment 25 is most probably wrong, and the perf improvements highlighted there are not the ones these patches will lead to.

2. on a complex web page (www.lemonde.fr)
  a. without the patches
     cold open: 780ms
     re-open: 440ms
  b. with the patches
     cold open: 850ms --> -9% regression
     re-open: 430ms   --> 2.3% win

The cold open test had quite a lot of variance on this complex web page, so I don't know how much we can trust it, but it seems to show that there's either a very small win, or actually an important regression on cold open.

I think one of these 2 things could explain it:
- the server-side processing on this page is quite important when the inspector starts, and so it takes most of the time when the panel opens up, and this dwarfs the wins made by lazy loading stuff in the UI
- or (or maybe and) the code paths that now lazy require dependencies are actually ran when the inspector starts. As we discussed yesterday, that ends up actually being slower than just non-lazily requiring those dependencies.
(Assignee)

Comment 40

6 months ago
This shaves off around 1.4ms for loading some of these middlewares that also have a lot of requires attached to them.
Attachment #9012594 - Flags: review?(pbrosset)
(Assignee)

Comment 41

6 months ago
This shaves off around 2.3ms from the TooltipsOverlay.addToView method
Attachment #9012603 - Flags: review?(pbrosset)
(Assignee)

Comment 42

6 months ago
Attachment #9012606 - Flags: review?(pbrosset)
(Assignee)

Updated

6 months ago
Attachment #9012606 - Flags: review?(pbrosset) → review?(jdescottes)
(Assignee)

Comment 43

6 months ago
I am seeing 2.5ms savings on the initial load of HTMLTooltip from the MenuButton.js in perfhtml
Attachment #9012608 - Flags: review?(jdescottes)
(Assignee)

Updated

6 months ago
Attachment #9012606 - Attachment is obsolete: true
Attachment #9012606 - Flags: review?(jdescottes)
(Assignee)

Comment 44

6 months ago
This saves us 0.5ms in the server.
Attachment #9012720 - Flags: review?(yzenevich)
(Assignee)

Comment 45

6 months ago
The point of this was to lazy load HTMLTooltip. I saw a combination of 1.1ms saved between markup and rules.
Attachment #9012725 - Flags: review?(jdescottes)
(Assignee)

Comment 46

6 months ago
Attachment #9012735 - Flags: review?(jdescottes)
(Assignee)

Updated

6 months ago
Attachment #9012725 - Flags: review?(jdescottes)
(Assignee)

Comment 47

6 months ago
Saw 1.1ms in savings
Attachment #9012735 - Attachment is obsolete: true
Attachment #9012735 - Flags: review?(jdescottes)
Attachment #9012736 - Flags: review?(jdescottes)
(Assignee)

Comment 48

6 months ago
When we add the tabs, we are already specifying the defaultTab that should be selected. We should avoid passing an id into toolsidebar.show() so we can avoid an unnecessary setState with the selected tab id, and just show the tabbar. 

I am seeing a 6-15ms shaving here.
Attachment #9012749 - Flags: review?(odvarko)
(Assignee)

Comment 49

6 months ago
Attachment #9012725 - Attachment is obsolete: true
Attachment #9012783 - Flags: review?(jdescottes)
(Assignee)

Comment 52

6 months ago
Attachment #9012790 - Flags: review?(odvarko)
(Assignee)

Updated

6 months ago
Attachment #9012790 - Attachment is obsolete: true
Attachment #9012790 - Flags: review?(odvarko)
Attachment #9012789 - Flags: review?(odvarko) → review+
Attachment #9012749 - Flags: review?(odvarko) → review+
Comment on attachment 9012319 [details] [diff] [review]
Part 16: Lazy load the TimeScale module in the animation reducer. [1.0]

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

Are you sure the TimeScale module isn't needed right from the start?
If the animation panel is loaded, then I *think* it executes a UPDATE_ANIMATIONS action, which will need this module. So lazy-loading it at that point might be slower.
Or maybe the reducer is always loaded when the inspector starts? If so, then that change makes sense.
Attachment #9012319 - Flags: review?(pbrosset) → review+
Attachment #9012337 - Flags: review?(pbrosset) → review+
Attachment #9012450 - Flags: review?(pbrosset) → review+
Attachment #9012452 - Flags: review?(pbrosset) → review+
Comment on attachment 9012455 [details] [diff] [review]
Part 20: Lazy load font-utils in the fonts reducer. [1.0]

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

R+ even though I have a comment below. Because no matter what the answer is, I won't need to re-review this change.

::: devtools/client/inspector/fonts/reducers/font-editor.js
@@ +15,5 @@
>  } = require("../actions/index");
>  
> +const { getStr } = require("../utils/l10n");
> +
> +loader.lazyRequireGetter(this, "parseFontVariationAxes", "devtools/client/inspector/fonts/utils/font-utils", true);

Same comment as in one of my previous reviews: parseFontVariationAxes is used in the UPDATE_EDITOR_STATE action, which sounds like an action we probably execute always, right from the start. So is it actually useful to lazy load this function? Or is it going to end up making it slower?
Attachment #9012455 - Flags: review?(pbrosset) → review+
Attachment #9012458 - Flags: review?(pbrosset) → review+
Attachment #9012461 - Flags: review?(pbrosset) → review+
Comment on attachment 9012594 [details] [diff] [review]
Part 23: Lazy load the redux middleware modules. [1.0]

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

I'm not familiar at all with this code.
Are you 100% confident that these dependencies aren't needed from the start? (not just of the inspector btw, but of any tool that uses react/redux/middlewares).
I just want to make sure that this doesn't end up being slower.
Moving this Honza for the final R+ on this as I just don't know enough here.
Attachment #9012594 - Flags: review?(pbrosset) → review?(odvarko)
Comment on attachment 9012603 [details] [diff] [review]
Part 24: Lazily get the preview tooltip to avoid loading HTMLTooltip. [1.0]

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

::: devtools/client/inspector/shared/tooltips-overlay.js
@@ +87,5 @@
>      this._isStarted = true;
>  
>      // For now, preview tooltip has to be instantiated on startup in order to
>      // call tooltip.startTogglingOnHover. Ideally startTogglingOnHover wouldn't be part
>      // of HTMLTooltip and offer a way to lazy load this tooltip.

Now, with your change, this comment is out of date. What changed that we can now avoid instantiating the tooltip on startup? The comment said that we needed to do this because of startTogglingOnHover. So, did something changed that we don't need to do this anymore?
If so, please adjust the comment.
Attachment #9012603 - Flags: review?(pbrosset)
Comment on attachment 9012784 [details] [diff] [review]
Part 30: Lazily get the highlighters-overlay only when needed. [1.0]

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

::: devtools/client/inspector/inspector.js
@@ +202,5 @@
>      return this.toolbox.highlighter;
>    },
>  
>    get highlighters() {
> +    console.trace();

Please remove this left-over debugging trace.

::: devtools/client/inspector/markup/views/element-editor.js
@@ +341,5 @@
>      this._displayBadge.textContent = displayType;
>      this._displayBadge.dataset.display = displayType;
>      this._displayBadge.title = DISPLAY_TYPES[displayType];
>      this._displayBadge.classList.toggle("active",
> +      !!this._highlighters &&

I find it a bit confusing that we access both the _highlighters and highlighters properties around the same lines.
It makes it harder for someone reading this for the first time to know which one does what, and when they should use one vs. the other.

I like that we have a nice getter to access this service, and that it hides away the complexity of getting it lazily if needed.
I don't like that we then expose this complexity to the rest of the file.

Could we avoid accessing _highlighters at all? Maybe by adding another getter that just returns _highlighters, whether its been initialized or not.
Of perhaps by just adding something like:

get isHighlightersReady() {
  return !!this._highlighters;
}

@@ +721,5 @@
>      this.markup.inspector.once("markupmutation", onMutations);
>    },
>  
>    startTrackingFlexboxHighlighterEvents() {
> +    if (!this._highlighters) {

Same comment for all these start/stop functions. You could replace those ifs with:
if (!this.isHighlightersReady) {
  return;
}

::: devtools/client/inspector/rules/views/text-property-editor.js
@@ +537,5 @@
>  
>      const gridToggle = this.valueSpan.querySelector(".ruleview-grid");
>      if (gridToggle) {
>        gridToggle.setAttribute("title", l10n("rule.gridToggle.tooltip"));
> +      if (this._ruleView._highlighters &&

everywhere else you wrote this.ruleView but wrote this._ruleView here. Probably a typo.

Other than that, same comment as in the previous file. It might be good for the rule-view to expose a getter like:

get isHighlightersReady() {}

or something like this.
Attachment #9012784 - Flags: review?(pbrosset)
Attachment #9012720 - Flags: review?(yzenevich) → review+

Comment 60

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c13ab5db4b
Part 26: Lazy load accessibility utils. r=yzen
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a9c00cdfef
Part 31: Lazy load the Reps in the layout panel. r=Honza
(Assignee)

Updated

6 months ago
Attachment #9012720 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012789 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012461 - Flags: checkin+

Comment 61

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df9e8ceddde
Part 22: Remove onUnderflow call in ArrowScrollBox init. r=pbro

Comment 62

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd66696c9f2
Part 2: Lazy load the UndoStack in the markup view. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f6a1098cf9e
Part 3: Lazy load the SlottedNodeContainer in the markup view. r=pbro
(Assignee)

Updated

6 months ago
Attachment #9012011 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012013 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012030 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #9012039 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #9012009 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #9012015 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012017 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012019 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012029 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012154 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012035 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012040 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012041 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012147 - Flags: checkin+

Comment 63

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef609de4cd5c
Part 4: Lazy load the modules in the TextEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f077ffe76ca5
Part 5: Lazy load modules in ElementEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9e187182a0f
Part 6: Lazy load the modules in the ElementContainer. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/4880d9571aa7
Part 7: Lazy load modules in the shared inspector utils. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb2b02dc4b47
Part 10: Lazy initialize the event details and image tooltip in the markup view. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f700e44ea63f
Part 11: Lazy load promiseWarn in the rules model. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/10143ae649e5
Part 12: Lazy load modules in RuleEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee129a8c5e00
Part 13: Lazy load modules in the TextPropertyEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f90da1a0201b
Part 14: Lazy load parseNamedDeclarations in Rule. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/c90d929f87f2
Part 15: Use the cached css properties in the rules view. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1dac4bffc06
Part 16: Lazy load the TimeScale module in the animation reducer. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbbf97a5802e
Part 17: Lazy load the redux middlewares in create-store. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/98c531e6f8d4
Part 19: Directly import getCssPath, getXpath and findCssSelector. r=pbro

Comment 64

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea5d67d3c2ae
Part 15 Followup: Fix eslint error. r=me CLOSED TREE
(Assignee)

Updated

6 months ago
Attachment #9012150 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012319 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012337 - Flags: checkin+

Comment 65

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d5a593adfda
Part 15 Followup followup: Lazy require isCssVariable. r=me CLOSED TREE
(Assignee)

Updated

6 months ago
Attachment #9012450 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012452 - Flags: checkin+

Comment 67

6 months ago
Backout by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14272a8b7f32
Backed out 16 changesets (bug 1494162, bug 1464192) for failures in test_css-logic-getCssPath.html CLOSED TREE
(Assignee)

Updated

6 months ago
Flags: needinfo?(gl)

Comment 68

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a73764c4497
Part 4: Lazy load the modules in the TextEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/94c2d8174021
Part 5: Lazy load modules in ElementEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5dc92d8a56c
Part 6: Lazy load the modules in the ElementContainer. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/757dc50f41b2
Part 7: Lazy load modules in the shared inspector utils. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe637f705fd2
Part 10: Lazy initialize the event details and image tooltip in the markup view. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5b33662379
Part 11: Lazy load promiseWarn in the rules model. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/29de4ed0f9e4
Part 12: Lazy load modules in RuleEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbf24eaaa6ec
Part 13: Lazy load modules in the TextPropertyEditor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/023390ca8ad2
Part 14: Lazy load parseNamedDeclarations in Rule. r=pbro

Comment 69

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61dd67d8c191
Part 15: Use the cached css properties in the rules view. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/81c88e41f78b
Part 16: Lazy load the TimeScale module in the animation reducer. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/63e5313bfc6d
Part 17: Lazy load the redux middlewares in create-store. r=pbro

Comment 70

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd32a2fbc66
Part 19: Directly import getCssPath, getXpath and findCssSelector. r=pbro

Comment 71

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6297b63aa220
Part 19 Follow up: Fix test failures in test_css-logic-getCssPath.html. r=me

Comment 72

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a69fe45e0de
Part 19: Backed out changeset 5fd32a2fbc66. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/409be898a58d
Part 19: Backed out changeset 6297b63aa220. r=me
(Assignee)

Updated

6 months ago
Attachment #9012458 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012749 - Flags: checkin+

Comment 74

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f0104010671
Part 29: Avoid calling toolsidebar show with an id when the default tab is already selected. r=Honza
(Assignee)

Comment 76

6 months ago
Attachment #9013093 - Flags: review?(pbrosset)
(Assignee)

Updated

6 months ago
Attachment #9012783 - Flags: review?(jdescottes)

Comment 80

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3fd96af0ca2
Part 19: Directly import getCssPath, getXpath and findCssSelector. r=pbro
Backed out for several clipboard and devtools failures on devtools/client e.g devtools/client/inspector/markup/test/browser_markup_shadowdom_copy_paths.js

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/be070bf5d08a5fa7efb83751b7b27d5f1269e5da

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception,usercancel,runnable&revision=d3fd96af0ca264b2637b88107420070ea9400700&group_state=expanded

e.g failure log clipboard: https://treeherder.mozilla.org/logviewer.html#?job_id=202409847&repo=mozilla-inbound&lineNumber=2905

[task 2018-09-29T20:32:13.038Z] 20:32:13     INFO - Opening the inspector
[task 2018-09-29T20:32:13.038Z] 20:32:13     INFO - Opening the toolbox
[task 2018-09-29T20:32:13.584Z] 20:32:13     INFO - GECKO(4063) | console.error: "Error while calling actor 'domnode's method 'getUniqueSelector'" "findCssSelector is not a function"
[task 2018-09-29T20:32:13.585Z] 20:32:13     INFO - GECKO(4063) | console.error: "getUniqueSelector@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/node.js:565:12\nhandler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1198:21\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1313:15\nreceiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:66:5\nMessageListener.receiveMessage*_addListener@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:40:5\nready@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:57:5\n_onConnection@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:892:5\nconnectToParent@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:313:12\nonConnect</<@resource://devtools/server/startup/frame.js:50:22\nonConnect<@resource://devtools/server/startup/frame.js:49:7\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\nMessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:72:5\n@resource://devtools/server/startup/frame.js:19:4\n"
[task 2018-09-29T20:32:13.585Z] 20:32:13     INFO - GECKO(4063) | console.error: "Protocol error (unknownError): findCssSelector is not a function"
[task 2018-09-29T20:32:13.663Z] 20:32:13     INFO - Toolbox opened and focused
[task 2018-09-29T20:32:13.699Z] 20:32:13     INFO - Find and expand the test-component shadow DOM host.
[task 2018-09-29T20:32:13.715Z] 20:32:13     INFO - Expand the shadow root
[task 2018-09-29T20:32:13.731Z] 20:32:13     INFO - Select the div under the shadow root
[task 2018-09-29T20:32:13.732Z] 20:32:13     INFO - Selecting the node for '[Front for domnode/server1.conn2.child1/domnode39]'
[task 2018-09-29T20:32:13.753Z] 20:32:13     INFO - GECKO(4063) | console.error: "Error while calling actor 'domnode's method 'getUniqueSelector'" "findCssSelector is not a function"
[task 2018-09-29T20:32:13.754Z] 20:32:13     INFO - GECKO(4063) | console.error: "getUniqueSelector@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/node.js:565:12\nhandler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1198:21\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1313:15\nreceiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:66:5\nMessageListener.receiveMessage*_addListener@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:40:5\nready@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:57:5\n_onConnection@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:892:5\nconnectToParent@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:313:12\nonConnect</<@resource://devtools/server/startup/frame.js:50:22\nonConnect<@resource://devtools/server/startup/frame.js:49:7\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\nMessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:72:5\n@resource://devtools/server/startup/frame.js:19:4\n"
[task 2018-09-29T20:32:13.757Z] 20:32:13     INFO - GECKO(4063) | console.error: "Protocol error (unknownError): findCssSelector is not a function"
[task 2018-09-29T20:32:13.764Z] 20:32:13     INFO - Check the copied values for the various copy*Path helpers
[task 2018-09-29T20:32:13.768Z] 20:32:13     INFO - GECKO(4063) | console.error: "Error while calling actor 'domnode's method 'getXPath'" "getXPath is not a function"
[task 2018-09-29T20:32:13.769Z] 20:32:13     INFO - GECKO(4063) | console.error: "getXPath@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/node.js:589:12\nhandler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1198:21\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1313:15\nreceiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:66:5\nMessageListener.receiveMessage*_addListener@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:40:5\nready@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:57:5\n_onConnection@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:892:5\nconnectToParent@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:313:12\nonConnect</<@resource://devtools/server/startup/frame.js:50:22\nonConnect<@resource://devtools/server/startup/frame.js:49:7\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\nMessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:72:5\n@resource://devtools/server/startup/frame.js:19:4\n"
[task 2018-09-29T20:32:13.770Z] 20:32:13     INFO - GECKO(4063) | console.error: "Protocol error (unknownError): getXPath is not a function"
[task 2018-09-29T20:32:18.803Z] 20:32:18     INFO - TEST-INFO | started process screentopng
[task 2018-09-29T20:32:19.102Z] 20:32:19     INFO - TEST-INFO | screentopng: exit 0
[task 2018-09-29T20:32:19.103Z] 20:32:19     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/markup/test/browser_markup_shadowdom_copy_paths.js | Timed out while polling clipboard for pasted data, got: waitForClipboard-known-value-0.47431154726991565 - 
[task 2018-09-29T20:32:19.103Z] 20:32:19     INFO - Stack trace:
[task 2018-09-29T20:32:19.103Z] 20:32:19     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:putAndVerify:1025
[task 2018-09-29T20:32:19.104Z] 20:32:19     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.promiseClipboardChange:1036
[task 2018-09-29T20:32:19.104Z] 20:32:19     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForClipboard:969
[task 2018-09-29T20:32:19.104Z] 20:32:19     INFO - chrome://mochitests/content/browser/devtools/client/shared/test/shared-head.js:waitForClipboardPromise/<:583
[task 2018-09-29T20:32:19.104Z] 20:32:19     INFO - chrome://mochitests/content/browser/devtools/client/shared/test/shared-head.js:waitForClipboardPromise:582
[task 2018-09-29T20:32:19.104Z] 20:32:19     INFO - chrome://mochitests/content/browser/devtools/client/inspector/markup/test/browser_markup_shadowdom_copy_paths.js:null:50
[task 2018-09-29T20:32:19.104Z] 20:32:19     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1093
[task 2018-09-29T20:32:19.105Z] 20:32:19     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084
[task 2018-09-29T20:32:19.105Z] 20:32:19     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:986
[task 2018-09-29T20:32:19.105Z] 20:32:19     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
[task 2018-09-29T20:32:19.105Z] 20:32:19     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-09-29T20:32:19.106Z] 20:32:19     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/markup/test/browser_markup_shadowdom_copy_paths.js | Uncaught exception - failed
[task 2018-09-29T20:32:19.106Z] 20:32:19     INFO - Leaving test bound 
[task 2018-09-29T20:32:19.106Z] 20:32:19     INFO - Removing tab.
[task 2018-09-29T20:32:19.106Z] 20:32:19     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2018-09-29T20:32:19.107Z] 20:32:19     INFO - Got event: 'TabClose' on [object XULElement].
[task 2018-09-29T20:32:19.107Z] 20:32:19     INFO - Tab removed and finished closing
[task 2018-09-29T20:32:19.107Z] 20:32:19     INFO - GECKO(4063) | MEMORY STAT | vsize 617MB | residentFast 272MB | heapAllocated 92MB
[task 2018-09-29T20:32:19.108Z] 20:32:19     INFO - TEST-OK | devtools/client/inspector/markup/test/browser_markup_shadowdom_copy_paths.js | took 6111ms

e.g devtools failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=202409743&repo=mozilla-inbound&lineNumber=6444

task 2018-09-29T20:26:44.439Z] 20:26:44     INFO - TEST-PASS | devtools/client/inspector/markup/test/browser_markup_flex_display_badge.js | No flexbox highlighter is shown. - 
[task 2018-09-29T20:26:44.441Z] 20:26:44     INFO - Toggling ON the flexbox highlighter from the flex display badge.
[task 2018-09-29T20:26:44.443Z] 20:26:44     INFO - Waiting for state predicate "state => state.flexbox.highlighted"
[task 2018-09-29T20:26:44.484Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_FLEXBOX"
[task 2018-09-29T20:26:44.563Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_OFFSET_PARENT"
[task 2018-09-29T20:26:44.579Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_LAYOUT"
[task 2018-09-29T20:26:44.728Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_FLEXBOX"
[task 2018-09-29T20:26:44.729Z] 20:26:44     INFO - GECKO(1429) | console.error: "Error while calling actor 'domnode's method 'getUniqueSelector'" "findCssSelector is not a function"
[task 2018-09-29T20:26:44.730Z] 20:26:44     INFO - GECKO(1429) | console.error: "getUniqueSelector@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/node.js:565:12\nhandler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1198:21\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1313:15\nreceiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:66:5\nMessageListener.receiveMessage*_addListener@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:40:5\nready@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:57:5\n_onConnection@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:892:5\nconnectToParent@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:313:12\nonConnect</<@resource://devtools/server/startup/frame.js:50:22\nonConnect<@resource://devtools/server/startup/frame.js:49:7\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\nMessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:72:5\n@resource://devtools/server/startup/frame.js:19:4\n"
[task 2018-09-29T20:26:44.751Z] 20:26:44     INFO - GECKO(1429) | console.error: "Protocol error (unknownError): findCssSelector is not a function"
[task 2018-09-29T20:26:44.752Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_OFFSET_PARENT"
[task 2018-09-29T20:26:44.771Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_LAYOUT"
[task 2018-09-29T20:26:44.805Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_OFFSET_PARENT"
[task 2018-09-29T20:26:44.842Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_LAYOUT"
[task 2018-09-29T20:26:44.979Z] 20:26:44     INFO - GECKO(1429) | console.log: "[DISPATCH] action type:" "UPDATE_FLEXBOX"
[task 2018-09-29T20:27:28.164Z] 20:27:28     INFO - TEST-INFO | started process screentopng
[task 2018-09-29T20:27:28.707Z] 20:27:28     INFO - TEST-INFO | screentopng: exit 0
[task 2018-09-29T20:27:28.707Z] 20:27:28     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/markup/test/browser_markup_flex_display_badge.js | Test timed out - 
[task 2018-09-29T20:27:28.707Z] 20:27:28     INFO - Removing tab.
[task 2018-09-29T20:27:28.707Z] 20:27:28     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2018-09-29T20:27:28.708Z] 20:27:28     INFO - Got event: 'TabClose' on [object XULElement].
[task 2018-09-29T20:27:28.709Z] 20:27:28     INFO - Tab removed and finished closing
[task 2018-09-29T20:27:28.711Z] 20:27:28     INFO - GECKO(1429) | MEMORY STAT | vsize 717MB | residentFast 311MB | heapAllocated 101MB
[task 2018-09-29T20:27:28.712Z] 20:27:28     INFO - TEST-OK | devtools/client/inspector/markup/test/browser_markup_flex_display_badge.js | took 45386ms
Flags: needinfo?(gl)
(Assignee)

Comment 82

6 months ago
Attachment #9012603 - Attachment is obsolete: true
Flags: needinfo?(gl)
Attachment #9013120 - Flags: review?(pbrosset)
(Assignee)

Updated

6 months ago
Attachment #9012783 - Attachment is obsolete: true
(Assignee)

Comment 84

6 months ago
This shaves off around 2.5-3ms by converting the initial state into a function so that we can lazily call getStr which allows us to avoid loading and reading the font-inspector.properties file until it is needed.
Attachment #9013135 - Flags: review?(rcaliman)
(Assignee)

Updated

6 months ago
Attachment #9013094 - Flags: review?(jdescottes)
(Assignee)

Updated

6 months ago
Attachment #9013094 - Flags: review?(jdescottes)
(Assignee)

Comment 85

6 months ago
Attachment #9013207 - Flags: review?(pbrosset)
(Assignee)

Updated

6 months ago
Attachment #9013135 - Flags: review?(rcaliman)
Comment on attachment 9012594 [details] [diff] [review]
Part 23: Lazy load the redux middleware modules. [1.0]

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

Looks good to me, thanks for the patch Gabriel!

Honza
Attachment #9012594 - Flags: review?(odvarko) → review+
Attachment #9012736 - Flags: review?(jdescottes) → review+
Attachment #9012608 - Flags: review?(jdescottes) → review+
Comment on attachment 9013094 [details] [diff] [review]
Part 35: Lazy load MenuButton, MenuItem, and MenuList components in ToolboxTabs. [1.0]

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

::: devtools/client/framework/components/ToolboxTabs.js
@@ +12,1 @@
>  const ToolboxTab = createFactory(require("devtools/client/framework/components/ToolboxTab"));

Should we lazy load ToolboxTab as well?
Attachment #9013094 - Flags: review?(jdescottes) → review+
Comment on attachment 9013095 [details] [diff] [review]
Part 37: Speed up getTypes in node-attribute-parser.js by using keys instead of looping over an array. [1.0]

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

::: devtools/client/shared/node-attribute-parser.js
@@ +306,2 @@
>      const containsAttribute = attributeName === typeData.attributeName ||
>                              typeData.attributeName === "*";

This line is weird, we don't have any attributeName in the array above that is *. So it looks like it's a feature we (or I, in that case) implemented but never used.
Maybe just remove this part of the condition.

And if you do, then remove the whole containsAttribute condition, because you're already just looping over the items of ATTRIBUTES[attributeName], so you're bound to just have these.

And therefore, also remove the attibuteName property from all of the objects in the list above. Since they already appear as keys of the ATTRIBUTE_TYPES object.
Attachment #9013095 - Flags: review?(pbrosset)
Comment on attachment 9013093 [details] [diff] [review]
Part 32: Lazy load Grid components. [1.0]

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

::: devtools/client/inspector/grids/components/Grid.js
@@ +15,5 @@
> +loader.lazyGetter(this, "GridList", function() {
> +  return createFactory(require("./GridList"));
> +});
> +loader.lazyGetter(this, "GridOutline", function() {
> +  return createFactory(require("./GridOutline"));

These 3 components are used in the render function, so that means they're actually needed as soon as the Grid component is needed. Unless there is no grid in the page.
Can you confirm that this is the reason you are lazy loading these components? And if so, please add a comment above the loader.lazyGetter calls to explain this.
Attachment #9013093 - Flags: review?(pbrosset) → review+
Attachment #9013098 - Flags: review?(pbrosset) → review+
Attachment #9013120 - Flags: review?(pbrosset) → review+
Comment on attachment 9013207 [details] [diff] [review]
Part 44: Lazy load findDOMNode. [1.0]

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

::: devtools/client/framework/components/ToolboxTabs.js
@@ +20,5 @@
>  loader.lazyGetter(this, "MenuList", function() {
>    return createFactory(require("devtools/client/shared/components/menu/MenuList"));
>  });
>  
> +loader.lazyRequireGetter(this, "findDOMNode", "devtools/client/shared/vendor/react-dom", true);

The 3 other components you changed look good, but this one is the only one that needs findDOMNode in onComponentDidMount. And the toolbox tabs are always visible. So this means that we're lazy requiring a module for nothing since it's actually needed when we start.
Attachment #9013207 - Flags: review?(pbrosset)
(Assignee)

Updated

6 months ago
Attachment #9012594 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012608 - Flags: checkin+

Comment 91

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50008b82b8ec
Part 23: Lazy load the redux middleware modules. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c00644cf715
Part 24: Lazily get the preview tooltip to avoid loading HTMLTooltip. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e25b5302003
Part 25: Lazy load the modules in HTMLTooltip. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ba9d86cee4
Part 28: Lazy initialize TooltipToggle in HTMLTooltip. r=jdescottes
(Assignee)

Updated

6 months ago
Attachment #9013120 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012736 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9012455 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #9013135 - Attachment is obsolete: true

Comment 92

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c68f9435308
Part 38: Use the cached css properties in the inspector. r=pbro
(Assignee)

Updated

6 months ago
Attachment #9013098 - Flags: checkin+

Comment 94

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67753603319f
Part 35: Lazy load MenuButton, MenuItem, and MenuList components in ToolboxTabs. r=jdescottes
(Assignee)

Comment 95

6 months ago
Comment on attachment 9013094 [details] [diff] [review]
Part 35: Lazy load MenuButton, MenuItem, and MenuList components in ToolboxTabs. [1.0]

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

::: devtools/client/framework/components/ToolboxTabs.js
@@ +12,1 @@
>  const ToolboxTab = createFactory(require("devtools/client/framework/components/ToolboxTab"));

No, we shouldn't lazy load things that will actually run since lazyGetter has a negative consequences of around 3% if the codepath is actually hit.
(Assignee)

Updated

6 months ago
Attachment #9013094 - Flags: checkin+
(Assignee)

Comment 97

6 months ago
Comment on attachment 9013093 [details] [diff] [review]
Part 32: Lazy load Grid components. [1.0]

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

Yes, we are lazily loading these components because we are assuming that grid is less likely to appear on the page.
(Assignee)

Comment 98

6 months ago
Attachment #9013207 - Attachment is obsolete: true
Attachment #9013494 - Flags: review?(pbrosset)
(Assignee)

Comment 101

6 months ago
Attachment #9013494 - Attachment is obsolete: true
Attachment #9013494 - Flags: review?(pbrosset)
Attachment #9013512 - Flags: review?(pbrosset)
Attachment #9013387 - Flags: review?(pbrosset) → review+
Comment on attachment 9013512 [details] [diff] [review]
Part 44: Use refs instead of findDOMNode. [2.0]

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

This looks good, I think, given my limited React experience. I'd feel more comfortable if you asked another person to take a quick look though.
Just 2 questions: are refs faster than findDOM? And, why did you not use createRef in the SplitBox component, like in the other ones?

::: devtools/client/shared/components/splitter/SplitBox.js
@@ +248,5 @@
>      return (
> +      dom.div(
> +        {
> +          className: classNames.join(" "),
> +          ref: div => {

I'm not familiar enough with React's ref to understand why you didn't use React.createRef in this component, and instead provided a function here.
Attachment #9013512 - Flags: review?(pbrosset) → review+
(Assignee)

Updated

6 months ago
Attachment #9013387 - Flags: checkin+

Comment 103

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4aea32bbca
Part 37: Speed up getTypes in node-attribute-parser.js by using keys instead of looping over an array. r=pbro

Comment 104

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/116b7931a196
Part 32: Lazy load Grid components. r=pbro
Comment on attachment 9013121 [details] [diff] [review]
Part 27: Lazy initialize the tooltip in AutocompletePopup. [3.0]

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

::: devtools/client/shared/autocomplete-popup.js
@@ +246,3 @@
>  
> +      this._list = null;
> +      this._listPadding = null;

nit: do we really need to nullify this?
Attachment #9013121 - Flags: review?(jdescottes) → review+

Comment 106

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce8b77ec351
Part 44: Use refs instead of findDOMNode. r=pbro
(Assignee)

Updated

6 months ago
Attachment #9013512 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9013510 - Flags: review?(bbirtles)

Comment 107

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef38ce41818
Part 27: Lazy initialize the tooltip in AutocompletePopup. r=jdescottes
(Assignee)

Updated

6 months ago
Attachment #9013121 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9013093 - Flags: checkin+
Comment on attachment 9013898 [details] [diff] [review]
Part 43: Lazy load createPortal to avoid creating the children of the MenuButton. [2.0]

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

::: devtools/client/shared/components/menu/MenuButton.js
@@ +75,5 @@
>  
>      this.state = {
>        expanded: false,
> +      // Whether or not the menu is initialized.
> +      isMenuInitialized: flags.testing || false,

I feel like this comment might be a little redundant ;)

If anything, it might be worth explaining what flags.testing is used for here.

@@ +87,5 @@
>      this.initializeTooltip();
> +
> +    if (!this.state.isMenuInitialized) {
> +      // Initialize the menu when the button is focused or moused over.
> +      for (const event of ["focus", "mousemove"]) {

Out of curiosity, any reason to prefer this over the requestIdleCallback approach we discussed?

The reason I ask is this approach is not going to help touch-screen users. Fortunately I don't think too many devs are triggering DevTools menus with touch-screens at this stage. (I mean I do, but I can live with the menu being less snappy in that case--I'm normally just glad if my fat fingers actually hit the right button.)

I assume we'll still get a focus event for touch screens so this won't completely break them... actually let me start up a build and see.
Attachment #9013889 - Flags: review?(jdescottes) → review+

Comment 117

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a30a201d8cd
Part 46: Lazy load openDocLink and assert in MeatballMenu. r=jdescottes
(Assignee)

Updated

6 months ago
Attachment #9013889 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9013916 - Flags: review?(pbrosset)
Comment on attachment 9013898 [details] [diff] [review]
Part 43: Lazy load createPortal to avoid creating the children of the MenuButton. [2.0]

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

r=me with the comment fix

::: devtools/client/shared/components/menu/MenuButton.js
@@ +87,5 @@
>      this.initializeTooltip();
> +
> +    if (!this.state.isMenuInitialized) {
> +      // Initialize the menu when the button is focused or moused over.
> +      for (const event of ["focus", "mousemove"]) {

I checked this locally and it seems like even if we call preventDefault() on the mousedown/touchstart event (a common technique for prevent buttons from stealing the focus) the menu still opens correctly on a touch device. So I think this is fine.

I'm still curious as to why you didn't go with the requestIdleCallback approach.
Attachment #9013898 - Flags: review?(bbirtles) → review+
Attachment #9014090 - Flags: review?(pbrosset) → review+
Attachment #9013892 - Flags: review?(pbrosset) → review+
Attachment #9013891 - Flags: review?(pbrosset) → review+
Attachment #9014252 - Flags: review?(rcaliman) → review+
Hi Gabriel, I think you should land what remains to be landed and mark this bug as fixed.
The types of patches you're doing now have less to do with lazyloading, and more with perceived performance. 
They are likely to be taking much more time than the previous ones, and could very much use their own dedicated bugs.
Also, we're getting close to the 64 nightly code freeze, and I wouldn't want a bug to span over 2 releases if some of its patches have already landed in the first one.

Comment 125

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6a5ee779a0d
Part 48: Lazy load getUnicodeUrl in ToolboxToolbar. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/465b5d63240a
Part 52: Don't show the empty message in the CssRuleView constructor. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/a06f75df15b5
Part 55: Remove traits for getCssPath and getXPath. r=rcaliman
(Assignee)

Updated

6 months ago
Attachment #9014252 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9013891 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9014090 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9013892 - Flags: checkin+
Attachment #9014249 - Flags: review?(jdescottes) → review+

Comment 126

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01ac27238f7f
Part 45: Lazy load Menu and MenuItem in TabBar. r=pbro

Comment 127

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd9ecb747f9d
Part 54: Stop loading codemirror.bundle.js in the markup. r=jdescottes
(Assignee)

Updated

6 months ago
Attachment #9014249 - Flags: checkin+
(Assignee)

Comment 128

6 months ago
Comment on attachment 9013898 [details] [diff] [review]
Part 43: Lazy load createPortal to avoid creating the children of the MenuButton. [2.0]

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

I did consider the requestIdleCallback approach after your initial review. It becomes a harder to see where the win for the requestIdleCallback is for the MenuButton since it still gets executed eventually but ideally it would execute after everything else is loaded (ie the inspector is fully loaded). That is not the case tho. The mousemove/focus initialization shows very clear win on DAMP. It is a bit hard to assess the benefits of requestIdleCallback with our current metrics. So, I am going with the clear winner for now.

Here are the 2 DAMPs for a comparison.

requestIdleCallback https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=b7feccb2a19100c217d7fde95d5727defc2934c5&newProject=try&newRevision=d57a6b3356e607c19763c316cb25fed3d0ca8d38&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

mousemove/focus initialization https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=b7feccb2a19100c217d7fde95d5727defc2934c5&newProject=try&newRevision=a5171ae968db10dd232ce159d852181b70cb0d06&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 129

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f12b32a5bb3c
Part 43: Lazy load createPortal to avoid creating the children of the MenuButton. r=birtles
Attachment #9014248 - Flags: review?(pbrosset) → review+
Comment on attachment 9014670 [details] [diff] [review]
Part 56: Reorder the function calls in deferredOpen to prioritize initializing the markup and sidebar panels. [1.0]

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

I'm amazed if try is still green after this! But this change makes a ton of sense anyway, so R+
Attachment #9014670 - Flags: review?(pbrosset) → review+
Comment on attachment 9014672 [details] [diff] [review]
Part 57: Set the node front before the components are initialized to unblock it from waiting until the markup is loaded before initializing. [1.0]

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

Looks good overall. I have one comment about simplifying the code to make it more readable.

::: devtools/client/inspector/inspector.js
@@ +293,5 @@
>      this.setupSearchBox();
>      await this.setupToolbar();
>  
> +    // Since we set the nodeFront before all the components are initialized. We perform
> +    // the actions of this.onNewSelection here.

This comment sounds a bit odd. It makes a lot of sense if you know what the code was before this change. But in a while, people will have a hard time understanding it.
In fact, it would be so much more natural to just call this.onNewSelection() right here. Could we try to do that?
Attachment #9014672 - Flags: review?(pbrosset)
(Assignee)

Updated

6 months ago
Attachment #9013898 - Flags: checkin+
(Assignee)

Updated

6 months ago
QA Contact: gl
Summary: Lazy load modules in the inspector → Improve the Inspector load speed

Comment 137

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/708061fd15ce
Part 53: Use Promise all to parallelize getting the page style, css properties and default node. r=pbro
(Assignee)

Updated

6 months ago
Attachment #9014248 - Flags: checkin+

Comment 138

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ae43bf692e
Part 56: Reorder the function calls in deferredOpen to prioritize initializing the markup and sidebar panels. r=pbro
Attachment #9014561 - Flags: review?(rcaliman) → review+
Attachment #9014669 - Flags: review?(rcaliman) → review+
Attachment #9014814 - Flags: review?(pbrosset) → review+

Comment 140

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a095b6e9b7d
Part 8: Lazy loadgetAngleValueInDegrees in color.js. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a52c822ebce
Part 47: Remove duplicate requires of addPseudoClassLock and removePseudoClassLock. r=rcaliman
(Assignee)

Updated

6 months ago
Attachment #9014561 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9014669 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9014670 - Flags: checkin+

Comment 141

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/289fbd180e44
Part 57: Set the node front before the components are initialized to unblock it from waiting until the markup is loaded before initializing. r=pbro
(Assignee)

Updated

6 months ago
Attachment #9014814 - Flags: checkin+
Comment on attachment 9014883 [details] [diff] [review]
Part 58: Remove the await call to block on markuploaded. [1.0]

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

Hmm, alright, sure, ok, as long as try is still green and the inspector still loads ok.
I'm uneasy about those major changes in the load sequence of the inspector, knowing that it's by nature quite racy, and that it's the panel people go to first the most. But if it helps load perf and still works, then great!
Attachment #9014883 - Flags: review?(pbrosset) → review+
Assignee: gl → nobody
Status: ASSIGNED → NEW
Assignee: nobody → gl
Status: NEW → ASSIGNED
QA Contact: gl
Comment on attachment 9012749 [details] [diff] [review]
Part 29: Avoid calling toolsidebar show with an id when the default tab is already selected. [1.0]

This one patch seems to have had a particular impact on performance:

== Change summary for alert #16535 (as of Fri, 05 Oct 2018 22:23:18 GMT) ==

Improvements:

 20%  damp complicated.inspector.open.DAMP windows7-32 opt e10s stylo      493.34 -> 395.02
 19%  damp complicated.inspector.open.DAMP windows7-32 pgo e10s stylo      446.66 -> 363.35
  9%  damp complicated.inspector.close.DAMP windows7-32 opt e10s stylo     58.30 -> 52.84
  4%  damp inspector.layout.open windows7-32 opt e10s stylo                422.07 -> 404.70
  4%  damp simple.inspector.open.DAMP windows7-32 opt e10s stylo           279.88 -> 269.52
  4%  damp cold.inspector.open.DAMP windows7-32 opt e10s stylo             526.28 -> 506.80

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=16535
Comment on attachment 9013898 [details] [diff] [review]
Part 43: Lazy load createPortal to avoid creating the children of the MenuButton. [2.0]

This particular changeset, or some other landed at the same time introduced such improvements to performance:
== Change summary for alert #16480 (as of Thu, 04 Oct 2018 17:46:51 GMT) ==

Regressions:

  2%  damp console.bulklog windows10-64-qr opt e10s stylo     120.94 -> 123.39

Improvements:

  8%  damp custom.jsdebugger.pause.DAMP windows10-64-qr opt e10s stylo         556.36 -> 510.65
  5%  damp simple.netmonitor.open.DAMP windows7-32 opt e10s stylo              234.09 -> 222.40
  4%  damp complicated.netmonitor.open.DAMP windows10-64-qr opt e10s stylo     295.39 -> 283.67
  3%  damp complicated.netmonitor.open.DAMP windows7-32 pgo e10s stylo         260.28 -> 251.31
  3%  damp simple.styleeditor.open.DAMP linux64-qr opt e10s stylo              262.09 -> 253.59
  3%  damp complicated.netmonitor.open.DAMP linux64-qr opt e10s stylo          315.17 -> 305.12
  3%  damp simple.styleeditor.open.DAMP linux64 pgo e10s stylo                 228.26 -> 221.06
  3%  damp simple.styleeditor.open.DAMP windows7-32 opt e10s stylo             228.25 -> 221.28
  3%  damp complicated.netmonitor.open.DAMP windows10-64 opt e10s stylo        289.07 -> 280.50
  3%  damp complicated.webconsole.open.DAMP windows10-64 opt e10s stylo        345.82 -> 335.87
  3%  damp simple.styleeditor.open.DAMP windows10-64 opt e10s stylo            233.57 -> 227.04
  3%  damp simple.styleeditor.open.DAMP windows10-64-qr opt e10s stylo         230.30 -> 224.06
  3%  damp complicated.netmonitor.open.DAMP windows7-32 opt e10s stylo         287.67 -> 279.87
  3%  damp console.openwithcache.open.DAMP linux64-qr opt e10s stylo           333.80 -> 324.78
  3%  damp complicated.webconsole.open.DAMP windows10-64-qr opt e10s stylo     353.40 -> 343.93
  3%  damp complicated.webconsole.open.DAMP windows7-32 opt e10s stylo         339.23 -> 330.38
  3%  damp console.openwithcache.open.DAMP windows10-64-qr opt e10s stylo      315.89 -> 307.66
  3%  damp complicated.webconsole.open.DAMP windows7-32 pgo e10s stylo         312.05 -> 304.00
  2%  damp simple.styleeditor.open.DAMP windows7-32 pgo e10s stylo             209.38 -> 204.81

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

Updated

6 months ago
Keywords: leave-open
(Assignee)

Updated

6 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Depends on: 1503175
You need to log in before you can comment on or make changes to this bug.