Closed
Bug 1012988
Opened 7 years ago
Closed 6 years ago
Connecting to an app increase its memory by 9MB
Categories
(DevTools :: Framework, defect)
DevTools
Framework
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.
Comment 1•7 years ago
|
||
Terrific findings Alex! Just make sure your lazy-loading work doesn't conflict with Eddy's worker debugging work.
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
(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/
![]() |
||
Updated•7 years ago
|
Whiteboard: [MemShrink]
Comment 5•7 years ago
|
||
(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)
![]() |
||
Updated•7 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P3]
Assignee | ||
Comment 6•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → poirot.alex
Assignee | ||
Updated•7 years ago
|
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+
Assignee | ||
Comment 8•7 years ago
|
||
(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
Assignee | ||
Comment 9•7 years ago
|
||
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
Comment 10•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1023a50167c5
Keywords: checkin-needed
Whiteboard: [MemShrink:P3] → [MemShrink:P3][fixed-in-fx-team]
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 11•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1023a50167c5
Whiteboard: [MemShrink:P3][fixed-in-fx-team] → [MemShrink:P3]
Assignee | ||
Comment 12•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Attachment #8435036 -
Flags: checkin+
Is this safe to close now that all dependencies are done? Great work, Alex! :D
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 14•6 years ago
|
||
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)
Assignee | ||
Comment 15•6 years ago
|
||
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)
Assignee | ||
Comment 16•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
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 18•6 years ago
|
||
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-
Assignee | ||
Updated•6 years ago
|
Attachment #8555342 -
Flags: review- → review?(rFobic)
Assignee | ||
Comment 19•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Attachment #8558170 -
Flags: review+
Comment 21•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(rFobic)
Assignee | ||
Comment 22•6 years ago
|
||
Comment on attachment 8555342 [details] [review] SDK pull request 1844 r+ on github and landed.
Attachment #8555342 -
Flags: review?(rFobic) → review+
Assignee | ||
Comment 23•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Updated•3 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•