Closed Bug 1259045 Opened 8 years ago Closed 8 years ago

Loader.jsm is loaded early, before browser-delayed-startup-finished

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Whiteboard: [btpp-backlog])

Attachments

(1 file, 2 obsolete files)

Bug 1248600 ensured that devtools code kicks in only once browser-delayed-startup-finished event fires.
It is somewhat true. Main modules are no longer loaded before that.
But Loader.jsm and some other modules are loaded before.
It is being loaded when devtools/shared/event-emitter.js is loaded as a JSM.
This module doesn't really depends on any other module, so there is no need for a full module loader. We can save a few cycles/memory when using this module early in firefox startup
Attached patch patch v1 (obsolete) — Splinter Review
Fixed a deprecated patch to event-emitter from browser/
and ensure not using Loader.jsm when used as a JSM.
Attached patch patch v2 (obsolete) — Splinter Review
Forgot to add the deprecated URL from browser/.
Attachment #8733881 - Attachment is obsolete: true
Attachment #8733884 - Flags: review?(jryans)
Comment on attachment 8733884 [details] [diff] [review]
patch v2

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

::: devtools/shared/event-emitter.js
@@ +36,5 @@
>  
>  this.EventEmitter = function EventEmitter() {};
>  module.exports = EventEmitter;
>  
> +// See comment in JSM module boiterplate when adding a new dependency.

Nit: boilerplate
Attachment #8733884 - Flags: review?(jryans) → review+
Attached patch patch v3Splinter Review
Attachment #8735791 - Flags: review+
Attachment #8733884 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/e56e76ec40cb0e8f11278a5e61414972b771e9fd
Bug 1259045 - Prevent loading Loader.jsm when using event-emitter.js as a JSM. r=jryans
Priority: -- → P3
Whiteboard: [btpp-backlog]
https://hg.mozilla.org/mozilla-central/rev/e56e76ec40cb
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
This changeset should have moved some computation from early in firefox startup, to after browser-delayed-startup-finished. Over the last months I landed several patch that better control when the devtools code runs and delay it later and later.
I'm interested to see if perf test has been improved or regressed on this changeset.

Do you know which test could be impacted? first paint?
Where can I see the values on mozilla-central changesets?
Flags: needinfo?(jmaher)
I would expect to see changes in ts_paint, and possible some of the xperf measurements which detect file i/o during startup.

There are 4 measures of ts_paint (opt, opt+e10s, pgo, pgo+e10s):
https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=%5Bmozilla-inbound,8a41738d21b57ca7c1c9ebba50ff2239be4091cc,1%5D&series=%5Bmozilla-inbound,619ab4c1115bc476b1b460a67288d28bdc272579,1%5D&series=%5Bmozilla-inbound,e5ca934c64e64394c7fe46a34fdcbce7b2bed374,1%5D&series=%5Bmozilla-inbound,e0b10e5fbf3a8d117e25d51bed07972bae7552ca,1%5D

you can see that opt/pgo pretty much track each other- we really only ship pgo, so lets focus on pgo/pgo+e10s:
pgo;
https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=%5Bmozilla-inbound,e5ca934c64e64394c7fe46a34fdcbce7b2bed374,1%5D&series=%5Bmozilla-inbound,e0b10e5fbf3a8d117e25d51bed07972bae7552ca,0%5D&series=%5Bmozilla-inbound,32d0b6f0e941cc3061d2ce9b4f36f035a7b30824,1%5D&series=%5Bmozilla-inbound,99bcbbb0fab2bb20d12e679236dc972a0c04b9e2,1%5D&series=%5Bmozilla-inbound,9a06a23a07b2ab578378120a18ec313d607a6f86,1%5D&zoom=1452008302219.1235,1459742349000,731.0706043310624,1080.651179502771

pgo+e10s:
https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=%5Bmozilla-inbound,e0b10e5fbf3a8d117e25d51bed07972bae7552ca,1%5D&series=%5Bmozilla-inbound,6983cd8da36ea94534fd299763eb952590cbbe7b,1%5D&series=%5Bmozilla-inbound,d7dc89dbe0930aad6b3789616401f924097bd192,1%5D&series=%5Bmozilla-inbound,3c1e5da6d0b28a281195f17268f8c04f5430ca76,1%5D

overall, I am not seeing anything significant in the ts_paint test.  looking at some of the xperf counters (win7 only):
https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=%5Bmozilla-inbound,a5cb3cabdfc19fcf9e4ce69ebc6e910ecc05a633,1%5D&series=%5Bmozilla-inbound,12945b0b398d773c03a01f2a8eb7006e2be4ca82,1%5D&series=%5Bmozilla-inbound,293ca37c03de1e5ff8c774385f682d22ef458b2a,1%5D

I don't see any big change.  Quite possibly this is a change which doesn't affect performance.
Flags: needinfo?(jmaher)
Thanks for all the links. That's disappointing. I looked at bug 1248600 landing date and don't see any change either. Bug 1248600 is even more significant.
I could easily imagine this bug isn't such a big win, but I would have imagine bug 1248600 to have at least a little shake.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.