Closed Bug 1012988 Opened 6 years ago Closed 6 years ago

Connecting to an app increase its memory by 9MB

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Whiteboard: [MemShrink:P3])

Attachments

(3 files, 1 obsolete file)

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.
Terrific findings Alex! Just make sure your lazy-loading work doesn't conflict with Eddy's worker debugging work.
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.
(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/
Depends on: 1013997
Whiteboard: [MemShrink]
Fabrice, is this bug useful for tarako?
Flags: needinfo?(fabrice)
(In reply to James Zhang from comment #4)
> Fabrice, is this bug useful for tarako?

No, because we don't enable devtools by default.
Flags: needinfo?(fabrice)
Whiteboard: [MemShrink] → [MemShrink:P3]
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
Assignee: nobody → poirot.alex
Attachment #8435036 - Flags: review?(jimb)
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! :)
Attachment #8435036 - Flags: review?(jimb) → review+
(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
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.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/1023a50167c5
Keywords: checkin-needed
Whiteboard: [MemShrink:P3] → [MemShrink:P3][fixed-in-fx-team]
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/1023a50167c5
Whiteboard: [MemShrink:P3][fixed-in-fx-team] → [MemShrink:P3]
Depends on: 1024157
Depends on: 988237
Depends on: 1098391
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.
Attachment #8435036 - Flags: checkin+
Is this safe to close now that all dependencies are done?  Great work, Alex! :D
Flags: needinfo?(poirot.alex)
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!
Flags: needinfo?(poirot.alex)
Attached file 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.
Attachment #8555342 - Flags: review?(rFobic)
Attached patch lazy load new stuff - v1 (obsolete) — Splinter Review
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.
Attachment #8555347 - Flags: review?(jryans)
Comment on attachment 8555347 [details] [diff] [review]
lazy load new stuff - v1

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

Looks great!
Attachment #8555347 - Flags: review?(jryans) → review+
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.
Attachment #8555342 - Flags: review?(rFobic) → review-
Attachment #8555342 - Flags: review- → review?(rFobic)
It looks like loader.lazyRequireGetter prefers absolute path for modules.
Ryans, I hope you don't mind if I carry over the r+ ;)
Attachment #8555347 - Attachment is obsolete: true
Attachment #8558170 - Flags: review+
Irakli, review ping!
Flags: needinfo?(rFobic)
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
Flags: needinfo?(rFobic)
Comment on attachment 8555342 [details] [review]
SDK pull request 1844

r+ on github and landed.
Attachment #8555342 - Flags: review?(rFobic) → review+
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 1134174
Keywords: leave-open
Depends on: 1146827
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.