1012988 - Connecting to an app increase its memory by 9MB

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

Today, when we want to debug an app, its process immediatly bumps by 9MB, no matter which actor you start using, even the simpliest possible one.

To get a sense of how big this memory bump is. The template app, which is the smallest possible app takes only 6MB. So devtools takes more than the whole web engine :/ (ok part of it may be shared with nuwa magic, but still...)

There is many ways to lower this memory consumption.
 * the most important one: lazy load, lazy load, lazy load. We do only few lazy loading, so that we end up loading almost always all dependencies on startup. Bug 993282 already tries to stop loading all actors on startup, so that we start loading actors (and their dependencies) only once we start using them. But that alone doesn't save much as minimal debugger server code already pull a lot of dependencies.
 * compartments for each SDK module cost a lot. bholley recently tried to use one compartment/global per JSM and saw that there is still a significant overhead. We are currently suffering for this overhead (more info in bug 989373)
 * less dependencies, more barebone js, less code: We are using some SDK modules that end up pulling many others. protocol.js for ex, pull many language helpers and require("devtools/server/protocol") introduce a bump of 930k.
Here is the impresive list of dependencies of protocol.js:
load: resource://gre/modules/devtools/server/protocol.js
load: resource://gre/modules/commonjs/sdk/core/heritage.js
load: resource://gre/modules/commonjs/sdk/event/target.js
load: resource://gre/modules/commonjs/sdk/event/core.js
load: resource://gre/modules/commonjs/sdk/core/namespace.js
load: resource://gre/modules/commonjs/sdk/lang/functional.js
load: resource://gre/modules/commonjs/sdk/util/deprecate.js
load: resource://gre/modules/commonjs/sdk/console/traceback.js
load: resource://gre/modules/commonjs/sdk/net/url.js
load: resource://gre/modules/commonjs/sdk/core/promise.js
Cu.import(resource://gre/modules/Promise.jsm)
load: resource://gre/modules/commonjs/sdk/util/object.js
load: resource://gre/modules/commonjs/sdk/util/array.js
Cu.import(resource://gre/modules/NetUtil.jsm)
load: resource://gre/modules/commonjs/sdk/preferences/service.js
load: resource://gre/modules/commonjs/sdk/timers.js
load: resource://gre/modules/commonjs/sdk/system/unload.js
load: resource://gre/modules/commonjs/sdk/system/events.js
load: resource://gre/modules/commonjs/sdk/platform/xpcom.js
load: resource://gre/modules/commonjs/sdk/util/uuid.js

I already have a big patch queue addressing most of these points, and all these efforts reduce the memory usage in child processes to 900k. (10x less, or "just protocol.js and its dependencies")

I started looking at memory consumption to suggest using the RDP for performance tools (firewatch) and also to give us a chance to have devtools working on next low memory devices.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Terrific findings Alex! Just make sure your lazy-loading work doesn't conflict with Eddy's worker debugging work.

Comment 2

[Alexandre Poirot [:ochameau]]

Sure thing, actually his work ease making dependencies clearer and helped me figure this out!
I already have patches, so I'll submit them and ask him for feedback.

Comment 3

[Alexandre Poirot [:ochameau]]

(In reply to Alexandre Poirot (:ochameau) from comment #0)
> startup. Bug 993282 already tries to stop loading all actors on startup

s/bug 993282/bug 988237/

Comment 4

[James Zhang (Spreadtrum)]

Fabrice, is this bug useful for tarako?

Comment 5

[[:fabrice] Fabrice Desré]

(In reply to James Zhang from comment #4)
> Fabrice, is this bug useful for tarako?

No, because we don't enable devtools by default.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Remove heritage dependency to packets.js.

A simple patch to start with.
Jim, I think you would easily agree on removing heritage dependency,
as it doesn't help much. We can use barebone Object.create/prototype...

This patch won't save much memory as-is.
But it will prevent loading heritage if we are using just old actors.
(Unfortunately, protocoljs is heavily based on heritage)

Any kB being freed on b2g is important as we are starting a debugger server in each app process.
Starting a server with a naive actor should be very cheap memory-wise!


https://tbpl.mozilla.org/?tree=Try&rev=e84036b3846e

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8435036 [details] [diff] [review]
Remove heritage dependency to packets.js.

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

This change seems fine!  Not sure why the diff is so terrible...

Sorry for adding the dependency to start with...  Will watch out for such things in the server going forward! :)

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #7)
> Comment on attachment 8435036 [details] [diff] [review]
> Remove heritage dependency to packets.js.
> 
> Review of attachment 8435036 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This change seems fine!  Not sure why the diff is so terrible...

indentation change + subtle parenthesis modification = sick diff

Comment 9

[Alexandre Poirot [:ochameau]]

I'm seeing orange on TBPL, one intermittent and another one that I assume being already broken on the changeset I'm based on:
42 INFO TEST-UNEXPECTED-FAIL | /tests/b2g/components/test/mochitest/test_sandbox_permission.html | Test timed out.
Bug 984274 - Intermittent test_sandbox_permission.html | Test timed out.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1023a50167c5

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1023a50167c5

Comment 12

[Alexandre Poirot [:ochameau]]

I'll be using this bug as a meta bug to strip down the memory usage of debugger server.
I would consider closing this meta bug, or at least be less active on it once we get under 1MB.

I just opened bug 1098391 to lazy load more stuff on the server initialization codepath.

The last patch, lazy load of actors, already droped down from 8MB to ~3MB (bug 988237)

We are still around 3MB today and this new patch will drop the memory down to 1.6MB.

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Is this safe to close now that all dependencies are done?  Great work, Alex! :D

Comment 14

[Alexandre Poirot [:ochameau]]

I reached the goal targetting ~1MB but I still have one more patch in my queue to strip it down even more.
We should also tweak toolkit/devtools/server/tests/unit/test_memory_footprint.js in order to reflect the new footprint!

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: SDK pull request 1844

Here is the last patch I has in my queue, to ensure that SDK doesn't kill our lazy getter defined here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/Loader.jsm#36
By accessing all loader predefined modules during loading instanciation.

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: lazy load new stuff - v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=edc4d818be6f
We regressed a bit with some new code,
here is some more lazy loads...

With these two patches we finally get under 1MB for a naive DebuggerServer \o/

We regressed to 1.8MB, but with these patches we get to 650KB!
(This is the amount of memory used to create the ContentActor in child process.
 i.e. more or less the cost of child.js)

Less memory also means less code to evaluate and be faster to resolve getAppActor request and faster toolbox creation.

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8555347 [details] [diff] [review]
lazy load new stuff - v1

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

Looks great!

Comment 18

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8555342 [details] [review]
SDK pull request 1844

I'm afraid there is a miss-match between changes and tests added. It also fails tests.

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: lazy load new stuff - v2

It looks like loader.lazyRequireGetter prefers absolute path for modules.
Ryans, I hope you don't mind if I carry over the r+ ;)

Comment 20

[Alexandre Poirot [:ochameau]]

Irakli, review ping!

Comment 21

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0d0ed6cee3270bfa23ee35f96d0eeb4d6d9df678
Bug 1012988 - Ensure that the loader doesn't force loader magic modules. r=gozala

https://github.com/mozilla/addon-sdk/commit/0a78838d7747a87e550547bb531b71b8653928b7
Merge pull request #1844 from ochameau/lazyModules

Bug 1012988 - Ensure that the loader doesn't force loader magic modules r=@gozala

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8555342 [details] [review]
SDK pull request 1844

r+ on github and landed.

Comment 23

[Alexandre Poirot [:ochameau]]

Considering this bug as done, the memory is now below 1MB. I'll file a followup to set a new threshold in the memory consumption test.