Last Comment Bug 672443 - [Jetpack] Too many compartments!
: [Jetpack] Too many compartments!
Status: RESOLVED FIXED
[MemShrink:P1]
: footprint
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P1 enhancement with 1 vote (vote)
: ---
Assigned To: Myk Melez [:myk] [@mykmelez]
:
Mentors:
: 672636 672637 (view as bug list)
Depends on: 646575 672636 672637 674492 677294
Blocks: 616850 mlk-fx8 671702
  Show dependency treegraph
 
Reported: 2011-07-18 22:34 PDT by Nicholas Nethercote [:njn]
Modified: 2012-02-02 07:14 PST (History)
26 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
xpcshell script to measure sandbox cost (2.25 KB, text/javascript)
2011-08-08 10:05 PDT, Alexandre Poirot [:ochameau] PTO, back on 1st
no flags Details
patch v1: should work, but doesn't seem to (1.95 KB, patch)
2011-11-16 15:47 PST, Myk Melez [:myk] [@mykmelez]
no flags Details | Diff | Splinter Review
patch v2: unhorked (1.90 KB, patch)
2012-01-09 14:49 PST, Myk Melez [:myk] [@mykmelez]
no flags Details | Diff | Splinter Review
patch v2 as a pull request (466 bytes, patch)
2012-01-09 16:26 PST, Myk Melez [:myk] [@mykmelez]
poirot.alex: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-07-18 22:34:26 PDT
Using the patch from bug 671159 I found that when I start Firefox with Bugzilla Tweaks installed, there are 40(!) system compartments (ie. compartments with a codebase of "[System Principal]".  This is just with about:memory?verbose loaded.  If I reload about:memory?verbose repeatedly, I see the number cycle between 40, 42, 44, 46, but not in any particular order.

I also tried Browse By Name, it only seems to create a single extra system compartment.

When viewing bugzilla pages with Bugzilla Tweaks enabled, the number of compartments would typically increase by up to 10 or 15 for each bug report opened in a new tab, but I couldn't see an obvious pattern predicting the number.

Is this an inherent problem with jetpack-based add-ons, or something specific to Bugzilla Tweaks?  khuey suspected the former.

Just in case your reaction is "lots of compartments, so what?" -- I saw that most of these compartments are method JITting a tiny-but-non-zero amount of code.  This adds up, because the minimum code chunk size for the jmit is 64KB.  (Doing less is difficult because VirtualAlloc doesn't allow sub-64KB aligned blocks.)
Comment 1 Alexandre Poirot [:ochameau] PTO, back on 1st 2011-07-19 01:45:59 PDT
Jetpack creates one Sandbox per module so if there is one compartment for each Sandbox, we can easily end up with a lot of compartments!
Most of these sandboxes use system principal. i.e. sandboxes for common js modules.
Then, when you use the page-mod API in order to have some content script applied on web pages, we are creating another set of sandboxes. This time, they are using the same principal than the web page they are targeting.

I'm currently working on bug 636145 in order to improve our sandboxing of common js module, as avoid giving system principal to all modules. But it appears to be quite challenging.

So if you think there is some platform capabilities we should try using in jetpack, do not hesitate to mention them.
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-19 07:34:04 PDT
(In reply to comment #0)
> Just in case your reaction is "lots of compartments, so what?" -- I saw that
> most of these compartments are method JITting a tiny-but-non-zero amount of
> code.  This adds up, because the minimum code chunk size for the jmit is
> 64KB.  (Doing less is difficult because VirtualAlloc doesn't allow sub-64KB
> aligned blocks.)

This is probably something to keep in mind with compartment-per-global too ...
Comment 3 Blake Kaplan (:mrbkap) 2011-07-19 09:31:12 PDT
(In reply to comment #2)
> This is probably something to keep in mind with compartment-per-global too

Compartment-per-global is blocked on making compartments lightweight enough to do compartment-per-global to avoid this very problem. In practice, that should mean moving the JIT caches out of the compartments and into the runtime.
Comment 4 :Ehsan Akhgari 2011-07-19 14:00:51 PDT
I've filed bug 672636 and bug 672637 for the two major issues on the Jetpack side here.  I'll drop the MemShrink tag on this bug for now, and we'll reevaluate when at least one of the dependencies gets fixed.
Comment 5 Nicholas Nethercote [:njn] 2011-07-19 16:14:40 PDT
I see two ways to fix this problem:

(a) create fewer compartments;
(b) make compartments cheaper.

Comment 3 is about (b);  it'll have to happen to get compartment-per-global.

The spin-off bugs (bug 672636 and bug 672637) are about (a).  If we achieve (b), (a) won't hurt but will it be truly necessary?  (Having said that, it'd be good to avoid bloating about:memory with lots of tiny uninteresting compartments.)
Comment 6 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2011-07-20 06:30:32 PDT
We probably could create compartment per add-on and use function wrappers for module instead of sandboxes. That's what for example nodejs does by default.
Comment 7 Nicholas Nethercote [:njn] 2011-07-20 21:40:13 PDT
Another fun data point:  if I open about:memory?verbose and techcrunch.com at start-up, I get 114 compartments.  Yikes.
Comment 8 Nicholas Nethercote [:njn] 2011-07-20 21:58:09 PDT
And if after opening techcrunch.com I open five articles from the front page in new tabs, I end up with 12 user compartments and 305 system compartments.

123(!) of these system compartments had identical stats:

│  ├──────140,037 B (00.03%) -- compartment([System Principal], 0x7fa6fde68000)
│  │      ├───69,632 B (00.02%) -- gc-heap
│  │      │   ├──39,064 B (00.01%) -- arena-unused
│  │      │   ├──17,600 B (00.00%) -- objects
│  │      │   ├──11,392 B (00.00%) -- shapes
│  │      │   ├───1,104 B (00.00%) -- arena-padding
│  │      │   ├─────408 B (00.00%) -- arena-headers
│  │      │   ├──────64 B (00.00%) -- strings
│  │      │   └───────0 B (00.00%) -- xml
│  │      ├───65,536 B (00.02%) -- mjit-code
│  │      ├────3,328 B (00.00%) -- object-slots
│  │      ├────1,381 B (00.00%) -- scripts
│  │      ├──────160 B (00.00%) -- string-chars
│  │      ├────────0 B (00.00%) -- mjit-data
│  │      ├────────0 B (00.00%) -- tjit-code
│  │      └────────0 B (00.00%) -- tjit-data
│  │               ├──0 B (00.00%) -- allocators-main
│  │               └──0 B (00.00%) -- allocators-reserve

So clearly we're getting huge numbers of identical compartments.
Comment 9 Alexandre Poirot [:ochameau] PTO, back on 1st 2011-07-21 11:13:55 PDT
Nick: the explosion of compartments number on techcrunch only appears when buzilla tweak is installed?

Here is the main module of this addon:
https://bitbucket.org/ehsan/bugzilla-tweaks/src/431a74ba2744/lib/main.js#cl-38
I don't think that's related to page-mod API as it will be triggered only on mozilla websites but I suspect context-menu API to add workers for each tab (or even worse, all content documents?)
Drew: I think that you are the best one for such question about context-menu?

ehsan: your bugs doesn't make much sense as there is no differences between internal and developer modules.

We are currently working on simplifying our module loader:
http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/2da6241694dc32a1
So that the code would be wayyy shorter/simplier/faster.
We had a long discussion about choosing the better platform API:
http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/f251a6810767aa7a/02b92fb28caf6829

I believe that we should first land a simplier module loader, then it will be easier to lower our compartments/sandbox uses.


Finally, we have content scripts, used by page-mod, context-menu APIs. 
They are using a really different code path and they have different requirements. I think the issue your are seing on techcrunch is related to them.
These scripts have to be executed with same principal than website and should not pollute web page context. So sandboxes appear as an obvious solution to use.
Comment 10 Drew Willcoxon :adw 2011-07-21 12:16:22 PDT
(In reply to comment #9)
> I don't think that's related to page-mod API as it will be triggered only on
> mozilla websites but I suspect context-menu API to add workers for each tab
> (or even worse, all content documents?)
> Drew: I think that you are the best one for such question about context-menu?

context-menu creates workers for each pair of menu items and content windows.
Comment 11 :Ehsan Akhgari 2011-07-21 12:55:14 PDT
(In reply to comment #9)
> ehsan: your bugs doesn't make much sense as there is no differences between
> internal and developer modules.

Well, in that case feel free to dupe one against the other.  :-)
Comment 12 Nicholas Nethercote [:njn] 2011-07-21 16:04:50 PDT
(In reply to comment #9)
> Nick: the explosion of compartments number on techcrunch only appears when
> buzilla tweak is installed?

Correct.  Without Bugzilla Tweaks I get numbers like 10--20 compartments.
Comment 13 Drew Willcoxon :adw 2011-07-21 16:31:58 PDT
(In reply to comment #10)
> context-menu creates workers for each pair of menu items and content windows.

BTW, Bugzilla Tweaks should restrict the sites it adds context menu items to, and therefore the number of workers created, by using URLContext(), just as it does with its page mods by using `include`.
Comment 14 :Ehsan Akhgari 2011-07-22 15:35:51 PDT
(In reply to comment #13)
> (In reply to comment #10)
> > context-menu creates workers for each pair of menu items and content windows.
> 
> BTW, Bugzilla Tweaks should restrict the sites it adds context menu items to,
> and therefore the number of workers created, by using URLContext(), just as it
> does with its page mods by using `include`.

IINM, it doesn't add context menu items to documents from other sites...
Comment 15 Drew Willcoxon :adw 2011-07-22 15:54:58 PDT
(In reply to comment #14)
> IINM, it doesn't add context menu items to documents from other sites...

It uses onBugzillaPage() in the urltest.js content script to test URLs in response to the "context" event, but because no URLContext() is specified, that test runs for any page when the context menu is invoked, regardless of its URL.  If it used URLContext() instead, its content scripts would only run on pages that matched the given URL context, not all pages.  So right now, when you invoke the context menu on TechCrunch with Bugzilla Tweaks installed, onBugzillaPage() runs, returns false, and so no Bugzilla Tweaks items are added to the menu.  If it used URLContext() instead, no Bugzilla Tweaks code would run at all for TechCrunch, because no workers would be created for that page.
Comment 16 Nicholas Nethercote [:njn] 2011-07-24 22:51:49 PDT
*** Bug 672637 has been marked as a duplicate of this bug. ***
Comment 17 Nicholas Nethercote [:njn] 2011-07-24 22:51:53 PDT
*** Bug 672636 has been marked as a duplicate of this bug. ***
Comment 18 Nicholas Nethercote [:njn] 2011-07-24 22:53:41 PDT
A tidbit from bug 672636 comment 1 (that bug has been marked as a dup of this bug):  an empty (jetpack-based) add-on causes five extra system compartments to be created!
Comment 19 :Ehsan Akhgari 2011-07-25 17:29:28 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > IINM, it doesn't add context menu items to documents from other sites...
> 
> It uses onBugzillaPage() in the urltest.js content script to test URLs in
> response to the "context" event, but because no URLContext() is specified,
> that test runs for any page when the context menu is invoked, regardless of
> its URL.  If it used URLContext() instead, its content scripts would only
> run on pages that matched the given URL context, not all pages.  So right
> now, when you invoke the context menu on TechCrunch with Bugzilla Tweaks
> installed, onBugzillaPage() runs, returns false, and so no Bugzilla Tweaks
> items are added to the menu.  If it used URLContext() instead, no Bugzilla
> Tweaks code would run at all for TechCrunch, because no workers would be
> created for that page.

I tried to follow the docs and do that, but I couldn't make it work.  Here's my patch: http://pastebin.mozilla.org/1282015
Comment 20 Drew Willcoxon :adw 2011-07-25 17:38:53 PDT
I haven't tested, but I don't think the match patterns in that patch are legal.  This page describes them:

https://addons.mozilla.org/en-US/developers/docs/sdk/1.0/packages/api-utils/docs/match-pattern.html

It's a little tricky.  Unfortunately URLContext doesn't currently support the regexp patterns described in that doc, but it should.
Comment 21 Nicholas Nethercote [:njn] 2011-07-26 19:40:01 PDT
Alexandre, Irakli:  This is an importatn bug.  Bug 636145 sounds like it will help here, but also sounds difficult.  The suggestion in comment 6 also sounds like a plausible fix.  Can you comment on if one or both are necessary?  And who would be a good person to work on the comment 6 suggestion, if it's necessary?
Comment 22 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2011-07-27 00:01:27 PDT
(In reply to comment #21)
> Alexandre, Irakli:  This is an importatn bug.  Bug 636145 sounds like it
> will help here, but also sounds difficult.  The suggestion in comment 6 also
> sounds like a plausible fix.  Can you comment on if one or both are
> necessary?  And who would be a good person to work on the comment 6
> suggestion, if it's necessary?

I don't think me or Alex are right persons to comment on whether or not suggestion in comment 6 it's necessary. It will for sure reduce amount of compartments that are created. Also please note that Bug 636145 is probably incompatible with comment 6 as it suggests creation of one compartment only in contrast to having per module compartments with different principals.

Since comment 6 suggests a big architectural change I think it's best to discuss it with Myk Melez (Jetpack Tech lead) and Dave Mason (Jetpack project manager) to have a specific bug with high priority.

Once we have a such a bug, I don't mind picking it up!
Comment 23 Brian Warner [:warner :bwarner] 2011-07-27 13:44:15 PDT
FWIW, what I care about is having modules protect their internal state
(including their references to dependent modules that they've imported), so
that when a reviewer sees main.js only do require("request"), they know that
main.js won't be able to reach inside request.js and get full XPCOM
authority. That's what will enable POLA and modular review. This includes
preventing one module from changing the primordials (Object, Number, etc) of
other modules. And it can't be too hard for module authors to do the right
thing: what their usual style of expression is, we should adjust the
platform/loader to let those modules be secure.

We might accomplish that by getting module authors to use lexically-scoped
state, and apply Caja-style restrictions (starting with ES5-Strict),
including freezing the primordials. Or maybe we put each module in a separate
compartment/sandbox/principal/etc and require authors to use other tools to
indicate how they want sharing to happen. As I understand it, our current
sandboxes give separate primordials to each module, so we can protect them
from each other without freezing, and therefore switching to
just-function-wrappers would also need to protect Object/Number somehow.
Comment 24 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2011-07-28 02:50:24 PDT
I've linked this bug to a module loader simplification work. My recent patch on that bug makes it possible to configure loader so that it either usese:

- compartment per add-on modules.
- compartment per module.

Also this does not affects compartment creation by page-mods, context-menus etc..
Comment 25 Nicholas Nethercote [:njn] 2011-08-02 17:25:46 PDT
Myk, you took assignment of this bug -- do you have plans for moving it forward?
Comment 26 Myk Melez [:myk] [@mykmelez] 2011-08-05 16:34:39 PDT
njn: yes, my plan for moving it forward is to dig into the capabilities of the platform and figure out what we're able to do while waiting for compartments to become cheap.

In particular, I'm going to investigate whether it's possible (or could be made possible) for sandboxes to share compartments, and I'm going to check if the function wrapper approach described in comment 6 achieves the degree of module isolation we want.
Comment 27 Alexandre Poirot [:ochameau] PTO, back on 1st 2011-08-08 10:05:29 PDT
Created attachment 551489 [details]
xpcshell script to measure sandbox cost

I took some time to measure sandbox memory usage with this xpcshell script that read resident memory (or malloc/allocated depending on Firefox version).

Sandboxes memory cost is falling down on each new release:
- Firefox 5.0 (20110705030204)
  250k/sandbox
- Firefox 6.0 (20110805030753)
  171k/sandbox
- Firefox 8.0 (20110808030804) Today's nightly
  29k/sandbox

If this script is right, a sandbox doesn't necessary cost 64KB at least.
Then I imagine that we can continue working on lowering these numbers?

I tried to use only one Sandbox for all modules by modifying Irakli's new module loader: https://github.com/mozilla/addon-sdk/pull/220
I end up saving around 2MB of resident memory on Firefox Nightly with Bugzilla tweak. This addon creates 76 additional compartments on firefox startup. 76*30 = 2280. This real world test match the sandbox memory measurement.
Comment 28 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-08 10:11:10 PDT
(In reply to Alexandre Poirot (:alex) from comment #27)
> Created attachment 551489 [details]
> xpcshell script to measure sandbox cost
> 
> I took some time to measure sandbox memory usage with this xpcshell script
> that read resident memory (or malloc/allocated depending on Firefox version).
> 
> Sandboxes memory cost is falling down on each new release:
> - Firefox 5.0 (20110705030204)
>   250k/sandbox
> - Firefox 6.0 (20110805030753)
>   171k/sandbox
> - Firefox 8.0 (20110808030804) Today's nightly
>   29k/sandbox
> 
> If this script is right, a sandbox doesn't necessary cost 64KB at least.
> Then I imagine that we can continue working on lowering these numbers?

A sandbox will cost 64KB once we JIT something inside it.

> I tried to use only one Sandbox for all modules by modifying Irakli's new
> module loader: https://github.com/mozilla/addon-sdk/pull/220
> I end up saving around 2MB of resident memory on Firefox Nightly with
> Bugzilla tweak. This addon creates 76 additional compartments on firefox
> startup. 76*30 = 2280. This real world test match the sandbox memory
> measurement.

How much memory does this save after using the browser for an hour or two?
Comment 29 Nicholas Nethercote [:njn] 2011-08-08 16:45:41 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #28)
> 
> A sandbox will cost 64KB once we JIT something inside it.

Moving the JIT caches into the JSRuntime (as per comment 3) will avoid this cost.

However, if the patch in bug 671702 lands that will make the JS heap quantum 64KB, up from the current 4KB.  With Bugzilla Tweaks installed I see lots of compartment gc-heaps with sizes like 68KB, 76KB, 80KB, and some as small as 24KB.
Comment 30 Wes Kocher (:KWierso) 2011-09-08 12:10:19 PDT
(Pushing all open bugs to the --- milestone for the new triage system)
Comment 31 Boris Zbarsky [:bz] 2011-10-19 12:37:48 PDT
Note that right now we only jit code for a sandbox compartment if that code is in a function called from outside the sandbox.  Code that runs during evalInSandbox is not jitted.  See bug 695806.
Comment 32 Myk Melez [:myk] [@mykmelez] 2011-11-02 15:15:37 PDT
Unassigning myself from bugs I am not actively working on.
Comment 33 Myk Melez [:myk] [@mykmelez] 2011-11-16 15:47:27 PST
Created attachment 575022 [details] [diff] [review]
patch v1: should work, but doesn't seem to

This patch seems like it should work, but I don't see a reduction in the number of compartments after applying it.

krizsa: am I using the API introduced by bug 677294 correctly?
Comment 34 Gabor Krizsanits [:krizsa :gabor] 2011-11-17 06:22:23 PST
(In reply to Myk Melez [:myk] [@mykmelez] from comment #33)
> krizsa: am I using the API introduced by bug 677294 correctly?

I think there is a simpler way to use it here, but the problem is not on your side, I debugged this and turned out that there is a bug in my patch for bug 677294, I'll continue this comment there.
Comment 35 Gabor Krizsanits [:krizsa :gabor] 2012-01-04 06:05:27 PST
Myk: the fixed patch has been landed so now it should work. 
https://hg.mozilla.org/mozilla-central/rev/941801e7439c
Comment 36 Myk Melez [:myk] [@mykmelez] 2012-01-09 14:49:59 PST
Created attachment 587163 [details] [diff] [review]
patch v2: unhorked

Here's an updated patch that applies cleanly.
Comment 37 Myk Melez [:myk] [@mykmelez] 2012-01-09 16:26:33 PST
Created attachment 587197 [details] [diff] [review]
patch v2 as a pull request

Here's the recent unhorked patch as an easily-mergeable pull request.  Requesting review from ochameau instead, as gozala asked him to review the pull request for bug 680821, which this patch also addresses.
Comment 38 Alexandre Poirot [:ochameau] PTO, back on 1st 2012-01-10 07:07:03 PST
Comment on attachment 587197 [details] [diff] [review]
patch v2 as a pull request

It breaks tests using helpers.js and introduce many warning messages.

Otherwise with this being fixed, it looks good.

Is gabor's patch finally landed for sameGroupAs, because I wasn't able to see any memory usage difference ?
Comment 39 Myk Melez [:myk] [@mykmelez] 2012-01-10 13:47:53 PST
Comment on attachment 587197 [details] [diff] [review]
patch v2 as a pull request

(In reply to Alexandre Poirot (:ochameau) from comment #38)
> It breaks tests using helpers.js and introduce many warning messages.

I added two changes to the pull request that should resolve these issues.


> Is gabor's patch finally landed for sameGroupAs, because I wasn't able to
> see any memory usage difference ?

Yes, it's landed.  If you run the annotator example with the changes and load about:memory?verbose, you should see many fewer compartments being created for modules.
Comment 40 Myk Melez [:myk] [@mykmelez] 2012-01-10 13:49:02 PST
Also, if this patch gets your review, please merge it too!  I'm in meetings this afternoon and might not get the chance before the merge to stabilization for the 1.5 release, and I'd like to get this in for 1.5 if possible.
Comment 41 Alexandre Poirot [:ochameau] PTO, back on 1st 2012-01-10 14:41:35 PST
Comment on attachment 587197 [details] [diff] [review]
patch v2 as a pull request

Works fine with very last nightly.
If you test with annotator example, it goes from 156 compartments down to 8 :o
Thanks gabor, thanks myk!

Speaking about memory it looks like it allowed to dropped around 20MB for this particular example.
Comment 42 [github robot] 2012-01-10 14:42:54 PST
Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0b626f74512212955c6fef44a917f89c6b907000
Merge pull request #325 from mykmelez/bug-672443-reuse-compartments

fix bug 672443 - reuse compartments between module sandboxes and set sandboxName to the module URI r=@ochameau
Comment 43 Alexandre Poirot [:ochameau] PTO, back on 1st 2012-01-10 14:46:51 PST
Nicolas, github robot automatically closed this bug. But feel free to reopen it or open more precise bugs if you think we should continue minimizing compartments number.

Note You need to log in before you can comment on or make changes to this bug.