Various helpers are used within firefox codepath of devtools. Most of them are pulled by jsonviewer and are naive helpers around xpcom. These helpers pull some other internal dependencies within Jetpack SDK. At the end, it introduces a significant overhead that can be seen on talos. attachment 8785984 [details] [diff] [review] removes these dependencies and the result is between 2 and 4% perf win; https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=cfdb7af3af2e&newProject=try&newRevision=3332f63169be&framework=1

This patch is just a WIP as it disables jsonviewer

Any SDK removal is more than welcome, and thanks for running the perf numbers. Why does this patch disable the jsonviewer?

I didn't had time to remove dependency on sdk/platform/xpcom from jsonview, which is pulling tons of other deps.

Does the talos improvement go away entirely if jsonview is re-enabled? Would be worth getting some improvement with a patch like this even if it's smaller with jsonview enabled.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=47e0584afe0a&newProject=try&newRevision=c65d6d2d7a84f74a4130f652c1ad9f425a30a942&framework=1&showOnlyImportant=0&showOnlyConfident=1 2% win on ts_paint 3-4% on sessionrestore 1-2% on damp The win has been reduces I think thanks to SDK loader perf fixes.

(In reply to Alexandre Poirot [:ochameau] from comment #8) > https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla- > central&originalRevision=47e0584afe0a&newProject=try&newRevision=c65d6d2d7a84 > f74a4130f652c1ad9f425a30a942&framework=1&showOnlyImportant=0&showOnlyConfiden > t=1 > > 2% win on ts_paint > 3-4% on sessionrestore > 1-2% on damp > > The win has been reduces I think thanks to SDK loader perf fixes. Wow, nice work!

Comment on attachment 8810916 [details] Bug 1302996 - Stop loading various useless SDK modules on early devtools startup. https://reviewboard.mozilla.org/r/93194/#review93668 This doesn't seem quite right to me yet... At the very least, more comments are needed about your assumptions. ::: devtools/client/framework/devtools-browser.js:713 (Diff revision 2) > }, > > /** > * All browser windows have been closed, tidy up remaining objects. > */ > - destroy: function () { > + destroy: function (unload = false) { Use an object arg here, so callers have to write out `{ unload: true }`, instead of an unnamed boolean. ::: devtools/client/framework/devtools-browser.js:713 (Diff revision 2) > }, > > /** > * All browser windows have been closed, tidy up remaining objects. > */ > - destroy: function () { > + destroy: function (unload = false) { What code path calls this _without_ passing `true`? It seems like there isn't one now? ::: devtools/client/framework/devtools.js:465 (Diff revision 2) > }, > > /** > - * Called to tear down a tools provider. > + * All browser windows have been closed, tidy up remaining objects. > */ > - _teardown: function DT_teardown() { > + destroy: function (unload = false) { Use an object arg here, so callers have to write out `{ unload: true }`, instead of an unnamed boolean. ::: devtools/client/framework/devtools.js:467 (Diff revision 2) > /** > - * Called to tear down a tools provider. > + * All browser windows have been closed, tidy up remaining objects. > */ > - _teardown: function DT_teardown() { > + destroy: function (unload = false) { > + // Do not cleanup everything during firefox shutdown, but only when > + // devtools are unloaded in the process of a reload. Expand this comment a bit more so that it's clear you mean reloading the DevTools via the contribution workflow, not reloading a page or something else. But wait... is this supposed to handle _only_ the reload case, or all loader destruction? It seems like it's all loader destruction at the moment... It seems like you aren't listening to Firefox shutdown at all anymore...?

Comment on attachment 8810917 [details] Bug 1302996 - Remove SDK dependencies for JSONView modules loaded on startup. https://reviewboard.mozilla.org/r/93196/#review93672 Thanks for working on this! Seems like a straightforward conversion away from SDK modules. ::: devtools/client/jsonview/converter-child.js:322 (Diff revision 2) > -}); > +}; > > function register() { > - if (!xpcom.isRegistered(service)) { > - xpcom.register(service); > + if (!registrar.isCIDRegistered(CLASS_ID)) { > + registrar.registerFactory(CLASS_ID, > + CLASS_DESCRIPTION, Nit: indentation seems arbitrary here...?

Comment on attachment 8810916 [details] Bug 1302996 - Stop loading various useless SDK modules on early devtools startup. https://reviewboard.mozilla.org/r/93194/#review94482 ::: devtools/client/framework/devtools-browser.js:713 (Diff revision 2) > }, > > /** > * All browser windows have been closed, tidy up remaining objects. > */ > - destroy: function () { > + destroy: function (unload = false) { sdk:loader:destroy is calling it with true from line 145. quit-application is calling it with false from line 755. ::: devtools/client/framework/devtools.js:467 (Diff revision 2) > /** > - * Called to tear down a tools provider. > + * All browser windows have been closed, tidy up remaining objects. > */ > - _teardown: function DT_teardown() { > + destroy: function (unload = false) { > + // Do not cleanup everything during firefox shutdown, but only when > + // devtools are unloaded in the process of a reload. This method is called by gDevtoolsBrowser.destroy. Itself is being called on : - sdk:loader:destroy (unload = true) - quit-application (unload = false) (so we still do listen for firefox shutdown) (see devtools-browser.js) sdk:loader:destroy is only called in addon reload codepath as that's the only one calling loader.destory via devtools-unload observer notification. I rephrased the comment, but do not hesitate to suggest tweaks. I put a comment in gDevtoolsBrowser.destroy.

Comment on attachment 8810916 [details] Bug 1302996 - Stop loading various useless SDK modules on early devtools startup. https://reviewboard.mozilla.org/r/93194/#review94536

Comment on attachment 8810916 [details] Bug 1302996 - Stop loading various useless SDK modules on early devtools startup. https://reviewboard.mozilla.org/r/93194/#review95420 Still needs some documentation / naming improvements. ::: devtools/client/framework/devtools-browser.js:143 (Diff revision 3) > for (let win of this._trackedBrowserWindows) { > this.updateCommandAvailability(win); > } > } > break; > + case "sdk:loader:destroy": Add comments to explain when this case would be triggered (when tools are reloaded for add-on workflow) ::: devtools/client/framework/devtools-browser.js:713 (Diff revision 3) > }, > > /** > * All browser windows have been closed, tidy up remaining objects. > */ > - destroy: function () { > + destroy: function (unload = false) { Use an object arg here. ::: devtools/client/framework/devtools-browser.js:713 (Diff revision 3) > }, > > /** > * All browser windows have been closed, tidy up remaining objects. > */ > - destroy: function () { > + destroy: function (unload = false) { Document the args. ::: devtools/client/framework/devtools.js:468 (Diff revision 3) > return toolbox.destroy().then(() => true); > }, > > /** > - * Called to tear down a tools provider. > + * All browser windows have been closed, tidy up remaining objects. > */ Document the args. ::: devtools/client/framework/devtools.js:469 (Diff revision 3) > }, > > /** > - * Called to tear down a tools provider. > + * All browser windows have been closed, tidy up remaining objects. > */ > - _teardown: function DT_teardown() { > + destroy: function (unload = false) { `unload` is a really ambiguous name. It's not clear at all what happens if true or false. Since it's passed along to distinguish shutdown vs. add-on reload, what about inverting the sense of it, so by default `destroy` kills everything (like you might assume it would). But if you call `destroy({ appShutdown: true })` (or something like this), it skips some steps. Use an object arg. ::: devtools/client/framework/devtools.js:472 (Diff revision 3) > - * Called to tear down a tools provider. > + * All browser windows have been closed, tidy up remaining objects. > */ > - _teardown: function DT_teardown() { > + destroy: function (unload = false) { > + // Do not cleanup everything during firefox shutdown, but only when > + // devtools are unloaded in the process of a reload. > + if (unload) { Is this check to skip some things on shutdown actually related to this bug? I thought you were focused on startup here?

Hum sorry but it looks like I r? the exact same patch again, I wanted to r? a new one with various things addressed!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18) > Comment on attachment 8810916 [details] > ::: devtools/client/framework/devtools.js:472 > (Diff revision 3) > > - * Called to tear down a tools provider. > > + * All browser windows have been closed, tidy up remaining objects. > > */ > > - _teardown: function DT_teardown() { > > + destroy: function (unload = false) { > > + // Do not cleanup everything during firefox shutdown, but only when > > + // devtools are unloaded in the process of a reload. > > + if (unload) { > > Is this check to skip some things on shutdown actually related to this bug? > I thought you were focused on startup here? I'm not changing any behavior here. That's because I'm merging _teardown and destroy here.

Sorry again for asking you to review the same patch, here is a new one with stuff actually being addressed!

Comment on attachment 8810916 [details] Bug 1302996 - Stop loading various useless SDK modules on early devtools startup. https://reviewboard.mozilla.org/r/93194/#review96066 Okay, there are enough comments that I can understand what's meant to happen by reading! :) At least one critical issue left, I think though! ::: devtools/client/framework/devtools-browser.js:738 (Diff revisions 3 - 4) > > - gDevTools.destroy(unload); > + gDevTools.destroy({ shuttingDown }); > }, > + > + onShutdown: function () { > + gDevToolsBrowser.destroy({ shuttingDown: false }); I am hoping this is supposed to be `true`, or else I am very confused! :P ::: devtools/client/framework/devtools-browser.js:762 (Diff revision 4) > }); > > gDevTools.on("toolbox-ready", gDevToolsBrowser._updateMenuCheckbox); > gDevTools.on("toolbox-destroyed", gDevToolsBrowser._updateMenuCheckbox); > > -Services.obs.addObserver(gDevToolsBrowser.destroy, "quit-application", false); > +Services.obs.addObserver(gDevToolsBrowser.onShutdown, "quit-application", false); Couldn't this just be another case block in the `observe` method on line 135, instead of using its own method? It's a bit strange to send some notifications through a common path, but then a different route for this one...

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #24) > ::: devtools/client/framework/devtools-browser.js:738 > (Diff revisions 3 - 4) > > > > - gDevTools.destroy(unload); > > + gDevTools.destroy({ shuttingDown }); > > }, > > + > > + onShutdown: function () { > > + gDevToolsBrowser.destroy({ shuttingDown: false }); > > I am hoping this is supposed to be `true`, or else I am very confused! :P Yes... scary typo ! > > ::: devtools/client/framework/devtools-browser.js:762 > (Diff revision 4) > > }); > > > > gDevTools.on("toolbox-ready", gDevToolsBrowser._updateMenuCheckbox); > > gDevTools.on("toolbox-destroyed", gDevToolsBrowser._updateMenuCheckbox); > > > > -Services.obs.addObserver(gDevToolsBrowser.destroy, "quit-application", false); > > +Services.obs.addObserver(gDevToolsBrowser.onShutdown, "quit-application", false); > > Couldn't this just be another case block in the `observe` method on line > 135, instead of using its own method? It's a bit strange to send some > notifications through a common path, but then a different route for this > one... Yes. I didn't saw that by doing a 1-1 refactoring. Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11c3f148ef8f72725a423e5bbf5bb90f915c7509

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/45bcf45afca2 Stop loading various useless SDK modules on early devtools startup. r=jryans https://hg.mozilla.org/integration/autoland/rev/8a97f217ebe8 Remove SDK dependencies for JSONView modules loaded on startup. r=jryans

A bunch of browser-chrome tests started failing around when this landed. https://treeherder.mozilla.org/logviewer.html#?job_id=7333464&repo=autoland Here's one look at them: https://treeherder.mozilla.org/#/jobs?repo=autoland&bugfiler&noautoclassify&group_state=expanded&fromchange=a105cf2d33a5643d63c516d5ecfe917d8eb5eeee&filter-searchStr=(bc&selectedJob=7313300 Here's more specifically Win7VMopt e10s-bc1: https://treeherder.mozilla.org/#/jobs?repo=autoland&bugfiler&noautoclassify&group_state=expanded&fromchange=a105cf2d33a5643d63c516d5ecfe917d8eb5eeee&filter-searchStr=01013d2740fbb158427a2a29b98aa98ae9485594&selectedJob=7333464 Backed out in https://hg.mozilla.org/integration/autoland/rev/fc08c9691328ad666158ffcb80caea747f7c98dc

Hum, the following test starts failing: http://searchfox.org/mozilla-central/source/browser/base/content/test/newtab/browser_newtab_bug1145428.js It asserts things within about:newtab. This is unrelated to this patch but the JSONView changes may have a performance impact and introduce a race in this test, which ends up crashing: 16:23:34 INFO - TEST-START | browser/base/content/test/newtab/browser_newtab_bug1145428.js 16:23:36 INFO - [Child 2956] ###!!! ABORT: Aborting on channel error.: file c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/ipc/glue/MessageChannel.cpp, line 2155 16:23:36 INFO - TEST-INFO | Main app process: exit 1 16:23:36 INFO - Buffered messages logged at 16:23:34 16:23:36 INFO - innerHeight,,884 16:23:36 INFO - innerWidth,,1030 16:23:36 INFO - Entering test bound setup 16:23:36 INFO - Leaving test bound setup 16:23:36 INFO - Entering test bound 16:23:36 INFO - Buffered messages finished 16:23:36 ERROR - TEST-UNEXPECTED-FAIL | browser/base/content/test/newtab/browser_newtab_bug1145428.js | application terminated with exit code 1 16:23:47 INFO - PROCESS-CRASH | browser/base/content/test/newtab/browser_newtab_bug1145428.js | application crashed [@ ClassHasEffectlessLookup] 16:23:47 INFO - Crash dump filename: c:\users\cltbld\appdata\local\temp\tmpey8yfx.mozrunner\minidumps\d3ae64f7-14c3-4cfd-ab06-99adb8750c80.dmp 16:23:47 INFO - Operating system: Windows NT 16:23:47 INFO - 6.1.7601 Service Pack 1 16:23:47 INFO - CPU: x86 16:23:47 INFO - GenuineIntel family 6 model 62 stepping 4 16:23:47 INFO - 8 CPUs 16:23:47 INFO - 16:23:47 INFO - GPU: UNKNOWN 16:23:47 INFO - 16:23:47 INFO - Crash reason: EXCEPTION_ACCESS_VIOLATION_READ 16:23:47 INFO - Crash address: 0xffffffffe5e5e5e5 16:23:47 INFO - Process uptime: 20 seconds 16:23:47 INFO - 16:23:47 INFO - Thread 0 (crashed) 16:23:47 INFO - 0 xul.dll!ClassHasEffectlessLookup [IonBuilder.cpp:8a97f217ebe8 : 8149 + 0x46] 16:23:47 INFO - eip = 0x61e66529 esp = 0x002ee794 ebp = 0x002ee794 ebx = 0x17511138 16:23:47 INFO - esi = 0x1b6846e8 edi = 0x17511590 eax = 0xe5e5e5e5 ecx = 0x0e69e208 16:23:47 INFO - edx = 0x1b6fc0a0 efl = 0x00010282 16:23:47 INFO - Found by: given as instruction pointer in context 16:23:47 INFO - 1 xul.dll!js::jit::IonBuilder::testSingletonPropertyTypes(js::jit::MDefinition *,jsid) [IonBuilder.cpp:8a97f217ebe8 : 8296 + 0x6] 16:23:47 INFO - eip = 0x61e9b3a3 esp = 0x002ee79c ebp = 0x002ee7c0 16:23:47 INFO - Found by: call frame info 16:23:47 INFO - 2 xul.dll!js::jit::IonBuilder::getPropTryConstant(bool *,js::jit::MDefinition *,jsid,js::TemporaryTypeSet *) [IonBuilder.cpp:8a97f217ebe8 : 11773 + 0xe] 16:23:47 INFO - eip = 0x61e826e5 esp = 0x002ee7c8 ebp = 0x002ee7e0 16:23:47 INFO - Found by: call frame info 16:23:47 INFO - 3 xul.dll!js::jit::IonBuilder::getElemTryGetProp(bool *,js::jit::MDefinition *,js::jit::MDefinition *) [IonBuilder.cpp:8a97f217ebe8 : 9411 + 0x14] 16:23:47 INFO - eip = 0x61e81250 esp = 0x002ee7e8 ebp = 0x002ee84f 16:23:47 INFO - Found by: stack scanning 16:23:47 INFO - 4 xul.dll!js::jit::IonBuilder::getElemTryGetProp(bool *,js::jit::MDefinition *,js::jit::MDefinition *) [IonBuilder.cpp:8a97f217ebe8 : 9411 + 0x14] 16:23:47 INFO - eip = 0x61e81250 esp = 0x002ee7f0 ebp = 0x002ee84f 16:23:47 INFO - Found by: stack scanning 16:23:47 INFO - 5 xul.dll!js::jit::IonBuilder::jsop_getelem() [IonBuilder.cpp:8a97f217ebe8 : 8995 + 0xd] 16:23:47 INFO - eip = 0x61e8afe5 esp = 0x002ee820 ebp = 0x002ee84f 16:23:47 INFO - Found by: stack scanning 16:23:47 INFO - 6 xul.dll!js::jit::IonBuilder::inspectOpcode(JSOp) [IonBuilder.cpp:8a97f217ebe8 : 2034 + 0x5] 16:23:47 INFO - eip = 0x61e88637 esp = 0x002ee858 ebp = 0x002ee938 16:23:47 INFO - Found by: call frame info with scanning 16:23:47 INFO - 7 mozglue.dll!arena_avail_tree_remove [jemalloc.c:8a97f217ebe8 : 3282 + 0x275] 16:23:47 INFO - eip = 0x6da636cf esp = 0x002ee868 ebp = 0x002ee938 16:23:47 INFO - Found by: stack scanning 16:23:47 INFO - 8 xul.dll!js::jit::IonBuilder::traverseBytecode() [IonBuilder.cpp:8a97f217ebe8 : 1548 + 0xb] 16:23:47 INFO - eip = 0x61e9bb08 esp = 0x002ee940 ebp = 0x17511000 16:23:47 INFO - Found by: call frame info 16:23:47 INFO - 9 xul.dll!js::jit::IonBuilder::build() [IonBuilder.cpp:8a97f217ebe8 : 926 + 0x7] 16:23:47 INFO - eip = 0x61e7adc5 esp = 0x002ee960 ebp = 0x002ee978 16:23:47 INFO - Found by: stack scanning 16:23:47 INFO - 10 xul.dll!js::jit::IonCompile [Ion.cpp:8a97f217ebe8 : 2236 + 0x7] 16:23:47 INFO - eip = 0x61e6ee7c esp = 0x002ee980 ebp = 0x002ee978 16:23:47 INFO - Found by: stack scanning

It looks like clasp happen to be null here: https://hg.mozilla.org/integration/autoland/annotate/8a97f217ebe82241078eb93fcb7540ffdef78109/js/src/jit/IonBuilder.cpp#l8145 ClassHasEffectlessLookup(const Class* clasp) Called from: https://hg.mozilla.org/integration/autoland/annotate/8a97f217ebe82241078eb93fcb7540ffdef78109/js/src/jit/IonBuilder.cpp#l8296 for (unsigned i = 0; i < types->getObjectCount(); i++) { TypeSet::ObjectKey* key = types->getObject(i); ... const Class* clasp = key->clasp(); if (!ClassHasEffectlessLookup(clasp) || ObjectHasExtraOwnProperty(compartment, key, id)) Jan, any guess? Unfortunately, that crash is not easy to reproduce as it seems to only happen on try while running BC1 on Windows 7 VM opt: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8a97f217ebe82241078eb93fcb7540ffdef78109

Hi Alexandre, sorry for the delay. I see a similar crash on OS X debug, https://treeherder.mozilla.org/logviewer.html#?job_id=7321764&repo=autoland#L2884 I'll try to reproduce this today, it looks really weird.

Actually let's lock this bug just to be sure.

I think this also happened on Linux x64: https://treeherder.mozilla.org/logviewer.html#?job_id=7318576&repo=autoland#L13576 - that bc3 orange was miss-starred. Unfortunately I've been unable to reproduce this locally on OS X and Linux. I did a full Try push with some Ion changes to see if it affects this: https://treeherder.mozilla.org/#/jobs?repo=try&author=jandemooij@gmail.com&group_state=expanded This should also run tests on more platforms.

Filed 1321374, unfortunately this will take a while :/

To fix the crash ? Did you figured it out ? How long before I can proceed with this patch ? Any workaround to at least have the tests green so that I can land ?

(In reply to Alexandre Poirot [:ochameau] from comment #36) > To fix the crash ? Did you figured it out ? Not 100% sure but I think so. > How long before I can proceed with this patch ? > Any workaround to at least have the tests green so that I can land ? I don't know of any :/ Hopefully this will be fixed on m-c within a week or two though. Sorry for the trouble!

Alexandre, the other bug has been fixed on m-c. Maybe you can rebase and do a Try push to see if this works now?

(In reply to Jan de Mooij [:jandem] from comment #38) > Alexandre, the other bug has been fixed on m-c. Maybe you can rebase and do > a Try push to see if this works now? Looks like it is green, thanks Jan!

https://hg.mozilla.org/integration/mozilla-inbound/rev/9098c0cb192578f74019366ef341169e6825ac74 Bug 1302996 - Stop loading various useless SDK modules on early devtools startup. r=jryans https://hg.mozilla.org/integration/mozilla-inbound/rev/43a718cdc5780332aaaa6f7ecad67035abbad691 Bug 1302996 - Remove SDK dependencies for JSONView modules loaded on startup. r=jryans

New talos run with better DAMP (with waitForSettle patch) https://treeherder.mozilla.org/perf.html#/comparesubtest?framework=1&newProject=try&newRevision=6d3abb301896adf8a85423fc53228991d252c044&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&originalProject=try&originalRevision=7778fe213e7101a551734d3faee29471786fccca&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac 2% win on DAMP e10s (netmon and inspector seem to benefit from this more than other tools) 4% on tps non-e10s

https://hg.mozilla.org/mozilla-central/rev/9098c0cb1925 https://hg.mozilla.org/mozilla-central/rev/43a718cdc578

(In reply to Alexandre Poirot [:ochameau] from comment #42) > New talos run with better DAMP (with waitForSettle patch) > 2% win on DAMP e10s (netmon and inspector seem to benefit from this more > than other tools) Wow, a very nice improvement for Netmonitor! Two questions: - is there any explanation why page reload should be faster? From the patch, I would guess that it should improve only the "open" time. - what's the improvement for waitForSettle? I don't see any change in the two patches in this bug. > 4% on tps non-e10s What does the "tps" test do? I'm familiar only with damp.

(In reply to Jarda Snajdr [:jsnajdr] from comment #44) > (In reply to Alexandre Poirot [:ochameau] from comment #42) > > New talos run with better DAMP (with waitForSettle patch) > > 2% win on DAMP e10s (netmon and inspector seem to benefit from this more > > than other tools) > > Wow, a very nice improvement for Netmonitor! Two questions: > - is there any explanation why page reload should be faster? From the patch, > I would guess that it should improve only the "open" time. Absolutely no idea, I think it is just DAMP randomness... This patch improves for sure the performance of Firefox startup and tab opening. And by doing that it should also speed up devtools as we are opening tabs during DAMP execution. But it has no effect on any devtools codepath. Like opening the toolbox, starting the server or anything alike. This patch prevents loading a bunch of SDK modules early during Firefox startup, but also when starting the child process for out of process tabs. It also significantly improve "multi e10s", where each new child process is going to be faster to start. > - what's the improvement for waitForSettle? I don't see any change in the > two patches in this bug. I was refering to jryans work from bug 1291815. > > > 4% on tps non-e10s > > What does the "tps" test do? I'm familiar only with damp. It is a firefox performance test around tab/content responsiveness: https://wiki.mozilla.org/Buildbot/Talos/Tests#tps

Should we try to uplift this to 52?

It could be worth uplifting, but only if bug 1321374 is also uplifted. Unless it covers a regression that doesn't exists in 52? That's because I expect it will triggers the same crash that backed this out in comment 29.

Bug 1321374 has already been uplifted to 51 and 52.

Ah ok, cool. I'll try to push it to try for 52 to see what tests say.

Looks like this got lost along the way somewhere? Do we still want this for 52?

Comment on attachment 8810916 [details] Bug 1302996 - Stop loading various useless SDK modules on early devtools startup. Approval Request Comment [Feature/Bug causing the regression]: I imagine the perf impact of devtools is not new. But it would have been significantly more since we turned jsonview on by default in FF53 (bug 1243951), which happen to be in the same release than this patch so we are good here. Still, this patch also improves firefox startup performances without opening devtools nor having jsonview turned on. https://hg.mozilla.org/mozilla-central/rev/9098c0cb1925 [User impact if declined]: Firefox as slow to startup as 51. [Is this code covered by automated tests?]: Yes, jsonview is tested as well as devtools startup. [Has the fix been verified in Nightly?]: I has been in nighly for quite a while now. [Needs manual test from QE? If yes, steps to reproduce]: This is a refactoring. Same feature/behavior but implemented differently. We tend to not do QE on this, but may be a smoketest would be justified. [List of other uplifts needed for the feature/fix]: I'm not aware of any regression. [Is the change risky?]: Yes it is non-trivial. [Why is the change risky/not risky?]: It is modifying startup code of devtools. In theory it could break devtools. [String changes made/needed]: no I've no idea if we are early or late in beta cycle, I'll let you make the call.

Comment on attachment 8810916 [details] Bug 1302996 - Stop loading various useless SDK modules on early devtools startup. Kinda feel late to take this in beta now. Too bad this wasn't nominated during aurora or early beta :/

Well, that's unfortunate.