Closed Bug 1302996 Opened 8 years ago Closed 7 years ago

A significant number of SDK modules are loaded during Firefox startup because of devtools

Categories

(DevTools :: Framework, enhancement, P3)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox51 wontfix, firefox52 wontfix, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Keywords: sec-other, Whiteboard: [adv-main53-][post-critsmash-triage])

Attachments

(2 files, 1 obsolete file)

Bug 1302996 - Stop loading various useless SDK modules on early devtools startup.
58 bytes, text/x-review-board-request
jryans
: review+
jcristau
: approval-mozilla-beta-
Details
Bug 1302996 - Remove SDK dependencies for JSONView modules loaded on startup.
58 bytes, text/x-review-board-request
jryans
: review+
Details 
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
Attached patch patch v1 (obsolete) — — Splinter Review 
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.
Severity: normal → enhancement
Priority: -- → P3
(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...?
Attachment #8810916 - Flags: review?(jryans) → review-
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...?
Attachment #8810917 - Flags: review?(jryans) → review+
Assignee: nobody → poirot.alex
Attachment #8791569 - Attachment is obsolete: true
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
Attachment #8810916 - Flags: review?(poirot.alex)
Attachment #8810916 - Flags: review?(poirot.alex) → review?(jryans)
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?
Attachment #8810916 - Flags: review?(jryans) → review-
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...
Attachment #8810916 - Flags: review?(jryans) → review+
(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
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
Flags: needinfo?(poirot.alex) → needinfo?(jdemooij)
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.
Group: core-security-release
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 :/
Flags: needinfo?(jdemooij)
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?
Flags: needinfo?(poirot.alex)
(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!
Flags: needinfo?(poirot.alex)
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
(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.
Flags: needinfo?(poirot.alex)
(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
Flags: needinfo?(poirot.alex)
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.
Depends on: 1321374
Keywords: sec-other
Looks like this got lost along the way somewhere? Do we still want this for 52?
status-firefox51: --- → wontfix
status-firefox52: --- → affected
Flags: needinfo?(poirot.alex)
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.
Flags: needinfo?(poirot.alex)
Attachment #8810916 - Flags: approval-mozilla-beta?
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 :/
Attachment #8810916 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Well, that's unfortunate.
status-firefox52: affected → wontfix
Whiteboard: [adv-main53-]
Flags: qe-verify-
Whiteboard: [adv-main53-] → [adv-main53-][post-critsmash-triage]
Group: core-security-release
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: