Closed Bug 1278625 Opened 8 years ago Closed 8 years ago

Remove lazy loading of modules that don't need it

Categories

(DevTools :: Framework, defect, P1)

Product:
DevTools
Component:
Framework
Type:
defect

Tracking

(firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Iteration:
51.2 - Aug 29
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: jlong, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(3 files)

Bug 1278625 - move XPCOMUtils use out of Tooltip.js;
58 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1278625 - remove some lazy loading from devtools;
58 bytes, text/x-review-board-request
jlong
: review+
Details
Bug 1278625 - move SimpleStringFront to specs/string.js;
58 bytes, text/x-review-board-request
ejpbruel
: review+
Details 
The current loader allows magical lazy loading via `loader.lazyRequireGetter` but we can't have that in content. I think many of these lazy loaders can just be converted to normal requires, but we should have a solution for lazily requiring modules. You can lazily require with webpack, but it's an async API instead of sync, but that's the only way to do it in content. I think we should move towards an async loader API.
Whiteboard: [devtools-html]
Blocks: devtools-html-3
Whiteboard: [devtools-html] → [devtools-html] [triage]
(In reply to James Long (:jlongster) from comment #0)
> I think we should move towards an async loader API.

Did you mean sync or async here?  If you do mean we should move to an async loader API, then are you actually okay with lazy requires as concept and you are mainly raising the issue that lazy require doesn't work with a sync loader?  Or do you want the lazy requires to go away?
Flags: needinfo?(jlong)
We could try rewriting all of the lazy requires into sync require() calls and see what DAMP says.  I don't think there's any argument to keep them besides as a performance optimization, and I don't have any idea how much they help.
(In reply to Brian Grinstead [:bgrins] from comment #2)
> We could try rewriting all of the lazy requires into sync require() calls
> and see what DAMP says.  I don't think there's any argument to keep them
> besides as a performance optimization, and I don't have any idea how much
> they help.

Yeah.  They have a bit more value on the server side to reduce memory for mobile devices.  For /devtools/client, removing them and checking perf impact seems worth a try.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> (In reply to James Long (:jlongster) from comment #0)
> > I think we should move towards an async loader API.
> 
> Did you mean sync or async here?  If you do mean we should move to an async
> loader API, then are you actually okay with lazy requires as concept and you
> are mainly raising the issue that lazy require doesn't work with a sync
> loader?  Or do you want the lazy requires to go away?

Oh, that was totally confusing. The normal `require` loader API should be sync. But we need to also expose as async API along the lines of `require("foo.js", function(foo) { ... })` when people really want async loading. I'm OK with the concept and we'll certainly want them in some of the core files that coordinate a lot of modules, and we don't want to pull everything in at once. In content, we have no way of pulling in things synchronously though (ok, we could use blocking AJAX requests, but let's avoid that...)

There are many places where I think we can switch to normal requires because they are immediately used afterwards anyway, that's what I meant. There are probably still some places that would benefit from lazy requires, but maybe for now we could get away with pulling everything in. We'll still use iframes to isolate the majority of stuff.
Flags: needinfo?(jlong)
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
Assignee: nobody → jlong
Status: NEW → ASSIGNED
Iteration: --- → 50.1
Priority: P2 → P1
Summary: Figure out alternative to lazy requires → Remove lazy loading of modules that don't need it
We're re-purposing this bug to remove lazy loading in certain core modules (like client/main.js) and where lazy loading isn't even needed. I'm hoping the majority of the places where it's used can just be converted to normal requires, and that will get us most of the way.
Iteration: 50.1 → 50.2
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
Assignee: jlong → nobody
Status: ASSIGNED → NEW
Iteration: 50.4 - Aug 1 → ---
Priority: P1 → P2
I think it's best to try removing lazy loading and see if it affects DAMP.
It seems like the simplest route forward.

One possible risk is that lazy loading might hide circular dependencies.
I don't know if our loader implementation handles these.


I do think it's possible to use our style of lazy loading with webpack.
The setup I am picturing is to use something like webpack's dynamic require
feature to implement lazyRequireGetter et al.  In this approach, the load
itself would not be lazy -- all the files would be in a single pack --
but it would rather let us handle circular dependencies in a way that
doesn't need async requires.  (This bit about not actually being lazy
wasn't clear at the meeting, sorry about that.)
By dynamic require you mean something like this?

function getName() {
  return "file.js";
}

require(getName());

Or some way of forcing an "expression". Webpack will warn about this. But I think you'll just hit the circular dependency at runtime instead. I could be wrong though.
The underlying technology needed for this:

https://webpack.github.io/docs/context.html

is exactly what's needed for lazyRequireGetter.
Testing a patch that just changes devtools/client/inspector for starters:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdd820f706f47e84d4fbe476517e4d8c166378f2
... and of course I didn't notice the bad edit.  Sigh.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b52c24223a25
IIUC these results look ok:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=498fd4aae629&newProject=try&newRevision=b52c24223a25da08a3df376032d0be130867cf8f&framework=1&showOnlyImportant=0

Also, I used the results of the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1266847#c3
(applied to an otherwise unmodified tree) to compare the list of requires modified in this patch to
what is actually loaded -- and every lazy require here is loaded during inspector startup.
So, at least as far as the inspector is concerned, I wouldn't expect this change to make any difference
(or perhaps a slight speedup due to running less code).

It's worth noting though that I didn't try this experiment by opening other tools and comparing
the resulting lists of loads.  It's possible that this might yield a slowdown; though DAMP seems
to indicate it's ok.
The other open question here is what happens if we apply this same transformation to
devtools/shared and devtools/client/{shared,framework}.  Changing the inspector is not sufficient.
I'm going to split the inspector patch into a separate bug to get it in sooner.
Depends on: 1292184
I don't think it's possible to remove all the lazy loads on the inspector startup path.
In particular the toolbox lazily loads all the tools; this is the spot where we'll want
to use multiple packs and webpack's async loading feature.

And, because we haven't converted the toolbox, it's a bit tricky to figure out exactly
what lazy loads do need to be removed.  So, I plan to convert a reasonable subset and try
that out; and then in phase 2 we can revisit this.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
Depends on: 1292574
I ignored a number of things for sanity:

* toolbox (e.g., definitions.js)
* menu.js
* all gcli command implementations
* performance fronts -- required by the toolbox but presumably not the inspector

The remaining issue I see is that Tooltip.js uses VariablesView:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/Tooltip.js#26

I tend to think this isn't reachable from the inspector.
Maybe it could be split out into a separate module.
https://reviewboard.mozilla.org/r/69694/#review66864

Going to redirect to Julian - this looks good to me overall.  The question I have is if the remaining logic in setVariableContent on the tooltip instance should be removed and migrated into setTooltipVariableContent.  I think the only place that function would make sense to call is from setTooltipVariableContent
Comment on attachment 8778376 [details]
Bug 1278625 - remove some lazy loading from devtools;

https://reviewboard.mozilla.org/r/69696/#review67182

Looks good, some of the code is just removed and not replaced with anything but I'm assuming it still works.
Attachment #8778376 - Flags: review?(jlong) → review+
Comment on attachment 8778375 [details]
Bug 1278625 - move XPCOMUtils use out of Tooltip.js;

Hm, I guess redirecting review in mozreview didn't set the flag here
Attachment #8778375 - Flags: review?(bgrinstead) → review?(jdescottes)
Comment on attachment 8778375 [details]
Bug 1278625 - move XPCOMUtils use out of Tooltip.js;

https://reviewboard.mozilla.org/r/69694/#review68642

Sorry, I totally missed the review request :/ Looks good to me, thanks!

I realized that the only caller of setTooltipVariableContent is never providing any argument after viewOptions. 
We should probably log a follow up to cleanup the helper and remove the unused arguments & code (unless you want to do it here)

::: devtools/client/shared/widgets/Tooltip.js:429
(Diff revision 1)
> -   * corresponding object actor, as specified in the remote debugging protocol.
> +   * function for @see setTooltipVariableContent.
>     *
> -   * @param {object} objectActor
> -   *        The value grip for the object actor.
> -   * @param {object} viewOptions [optional]
> -   *        Options for the variables view visualization.
> +   * @param {node} content
> +   *        The node to install
> +   */
> +  setVariableContent: function (node) {

As bgrins suggested, let's remove this method and move its logic in your new helper.

::: devtools/client/shared/widgets/tooltip/VariableContentHelper.js:31
(Diff revision 1)
> + * @param {object} controllerOptions [optional]
> + *        Options for the variables view controller.
> + * @param {object} relayEvents [optional]
> + *        A collection of events to listen on the variables view widget.
> + *        For example, { fetched: () => ... }
> + * @param {boolean} reuseCachedWidget [optional]

nit: jsdoc not in sync with code: reuseCacheWidget does not exist, instead extraButtons should be documented. (or removed, see my overall comment)
Attachment #8778375 - Flags: review?(jdescottes) → review+
Comment on attachment 8778375 [details]
Bug 1278625 - move XPCOMUtils use out of Tooltip.js;

https://reviewboard.mozilla.org/r/69694/#review68642

I looked and I think all the arguments are used.
It's a bit hard to read because the call is so very long:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/views/variable-bubble-view.js#201-228
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
Comment on attachment 8781627 [details]
Bug 1278625 - move SimpleStringFront to specs/string.js;

https://reviewboard.mozilla.org/r/72010/#review69900

:ejpbruel r+'d this on irc.
Attachment #8781627 - Flags: review+
Comment on attachment 8781627 [details]
Bug 1278625 - move SimpleStringFront to specs/string.js;

https://reviewboard.mozilla.org/r/72010/#review69960

This needed some updates due to failed tests and probably needs re-review.
Attachment #8781627 - Flags: review?(ttromey)
Attachment #8781627 - Flags: review?(ttromey)
Comment on attachment 8781627 [details]
Bug 1278625 - move SimpleStringFront to specs/string.js;

https://reviewboard.mozilla.org/r/72010/#review69898

Jup, this is what we discussed on irc. I don't have much to add, other than perhaps adding a comment as to why we've moved this code to specs (in case someone ever tries to move it back).
Attachment #8781627 - Flags: review?(ejpbruel) → review+
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/907dafc7ea66
move XPCOMUtils use out of Tooltip.js; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/6c0ee924a16b
remove some lazy loading from devtools; r=jlongster
https://hg.mozilla.org/integration/autoland/rev/987a4d3e9f0d
move SimpleStringFront to specs/string.js; r=ejpbruel
https://hg.mozilla.org/mozilla-central/rev/907dafc7ea66
https://hg.mozilla.org/mozilla-central/rev/6c0ee924a16b
https://hg.mozilla.org/mozilla-central/rev/987a4d3e9f0d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: