Last Comment Bug 844180 - Share zones across SDK modules
: Share zones across SDK modules
Status: RESOLVED FIXED
[MemShrink:P3]
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: ---
Assigned To: Alexandre Poirot [:ochameau]
:
Mentors:
Depends on: Zones
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-22 10:06 PST by Dave Townsend [:mossop]
Modified: 2013-05-22 14:20 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Pull request 866 (165 bytes, text/html)
2013-03-17 10:12 PDT, Alexandre Poirot [:ochameau]
no flags Details
Pull request 942 (165 bytes, text/html)
2013-04-15 09:03 PDT, Alexandre Poirot [:ochameau]
dtownsend: review+
Details

Description Dave Townsend [:mossop] 2013-02-22 10:06:02 PST
Bug 759585 is adding a sameZoneAs option to the sandbox constructor, looks like roughly the same semantics as sharing compartments used to have. We should make all our sandboxes share zones to save memory.

Alex, sounds like your bag, but this is lower priority than any remaining work on the API landings and any help you can bring to the private browsing stuff in 841823.
Comment 1 Alexandre Poirot [:ochameau] 2013-03-17 10:12:11 PDT
Created attachment 725895 [details]
Pull request 866

Before the patch:
63,674,552 B (100.0%) -- explicit
...
│  │  │  ├─────239,636 B (00.38%) -- compartment([System Principal], resource://gre/modules/commonjs/sdk/deprecated/traits.js (from: resource://gre/modules/XPIProvider.jsm -> jar:file:///c:/users/alex/appdata/local/temp/tmpyfmuix.mozrunner/extensions/anonid0-reddit-panel@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js:180))
│  │  │  │     ├──172,032 B (00.27%) -- gc-heap
│  │  │  │     │  ├──106,224 B (00.17%) ── unused-gc-things
│  │  │  │     │  ├───34,480 B (00.05%) -- objects
│  │  │  │     │  │   ├──25,760 B (00.04%) ── cross-compartment-wrapper
│  │  │  │     │  │   └───8,720 B (00.01%) ── function
│  │  │  │     │  ├───20,384 B (00.03%) ── sundries
│  │  │  │     │  └───10,944 B (00.02%) ── shapes/tree/global-parented
│  │  │  │     ├───32,768 B (00.05%) ── cross-compartment-wrapper-table
│  │  │  │     ├───21,972 B (00.03%) ── other-sundries
│  │  │  │     └───12,864 B (00.02%) ── objects-extra/slots

After:
54,513,142 B (100.0%) -- explicit
...
│  │  │  ├─────130,684 B (00.24%) -- compartment([System Principal], resource://gre/modules/commonjs/sdk/deprecated/traits.js (from: resource://gre/modules/XPIProvider.jsm -> jar:file:///c:/users/alex/appdata/local/temp/tmpdsdxel.mozrunner/extensions/anonid0-reddit-panel@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js:180))
│  │  │  │     ├───65,192 B (00.12%) -- gc-heap
│  │  │  │     │   ├──34,480 B (00.06%) -- objects
│  │  │  │     │   │  ├──25,760 B (00.05%) ── cross-compartment-wrapper
│  │  │  │     │   │  └───8,720 B (00.02%) ── function
│  │  │  │     │   ├──19,768 B (00.04%) ── sundries
│  │  │  │     │   └──10,944 B (00.02%) ── shapes/tree/global-parented
│  │  │  │     ├───32,768 B (00.06%) ── cross-compartment-wrapper-table
│  │  │  │     ├───19,860 B (00.04%) ── other-sundries
│  │  │  │     └───12,864 B (00.02%) ── objects-extra/slots

Looks like a big win!
Comment 2 Bill McCloskey (:billm) 2013-03-17 10:30:42 PDT
Could you explain what this patch does? I didn't do a good job of explaining what's needed in bug 759585, and I don't know enough about how things worked before CPG to understand comment 0. Here's my understanding of the situation:

- Now that the zones patch has landed, all sandboxes will be created in the same "system zone" by default, so they'll share a lot of GC resources.
- If a sandbox is not tied to any particular page, then that's the behavior we want.
- However, if a sandbox is expected to die when a page dies, then we want to stick it in the same zone as the page. So we would want to pass some object from the page as the sameZoneAs parameter.

Bobby, does that sound right? Is that what the patch does?

Also, did you take the "before" measurement using a build where the zones patch was applied?
Comment 3 Alexandre Poirot [:ochameau] 2013-03-17 10:59:44 PDT
Ah ok, I thought your patch was only doing something when you pass sameZoneAs.
So if I follow you correctly, as SDK modules are using system principal, we shouldn't have to do anything to share the same zone...

The "before" was using a nightly without platform zone patch.

Here is the with the platform patch but without the sdk patch I just submitted:
│  │  │  ├─────130,684 B (00.22%) -- compartment([System Principal], resource://gre/modules/commonjs/sdk/deprecated/traits.js (from: resource://gre/modules/XPIProvider.jsm -> jar:file:///c:/users/alex/appdata/local/temp/tmp2_mxof.mozrunner/extensions/anonid0-reddit-panel@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js:180))
│  │  │  │     ├───65,192 B (00.11%) -- gc-heap
│  │  │  │     │   ├──34,480 B (00.06%) -- objects
│  │  │  │     │   │  ├──25,760 B (00.04%) ── cross-compartment-wrapper
│  │  │  │     │   │  └───8,720 B (00.01%) ── function
│  │  │  │     │   ├──19,768 B (00.03%) ── sundries
│  │  │  │     │   └──10,944 B (00.02%) ── shapes/tree/global-parented
│  │  │  │     ├───32,768 B (00.06%) ── cross-compartment-wrapper-table
│  │  │  │     ├───19,860 B (00.03%) ── other-sundries
│  │  │  │     └───12,864 B (00.02%) ── objects-extra/slots

It confirms what you just said. So we don't have to do anything for module sandboxes, but we will have to pass sameZoneAs for content script sandboxes, that aren't using system principal but page principal instead.
Comment 4 Bobby Holley (PTO through June 13) 2013-03-17 21:57:19 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #3)
> It confirms what you just said. So we don't have to do anything for module
> sandboxes, but we will have to pass sameZoneAs for content script sandboxes,
> that aren't using system principal but page principal instead.

Correct, unless the GC characteristics of the addon SDK would warrant a separate zone. For example, if they accumulated a lot of garbage, it would be advantageous to be able to GC them separately from the rest of the system zone (all the JSMs and such).
Comment 5 Alexandre Poirot [:ochameau] 2013-04-15 08:11:55 PDT
(In reply to Bobby Holley (:bholley) from comment #4)
> Correct, unless the GC characteristics of the addon SDK would warrant a
> separate zone. For example, if they accumulated a lot of garbage, it would
> be advantageous to be able to GC them separately from the rest of the system
> zone (all the JSMs and such).

I can easily think about addon disabling/uninstall, where we are going to trash all module sandboxes. But is it really worth? Is it going to introduce additional cost in code being called between system global zone and addons zones? Or some more memory cost due to wrappers? Addon disabling isn't something that happen often...
Comment 6 Bobby Holley (PTO through June 13) 2013-04-15 08:17:39 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #5)
> (In reply to Bobby Holley (:bholley) from comment #4)
> > Correct, unless the GC characteristics of the addon SDK would warrant a
> > separate zone. For example, if they accumulated a lot of garbage, it would
> > be advantageous to be able to GC them separately from the rest of the system
> > zone (all the JSMs and such).
> 
> I can easily think about addon disabling/uninstall, where we are going to
> trash all module sandboxes. But is it really worth? Is it going to introduce
> additional cost in code being called between system global zone and addons
> zones? Or some more memory cost due to wrappers? Addon disabling isn't
> something that happen often...

Yeah, they should probably go in the system zone. Content-scripts should definitely go in the associated content zone, though.
Comment 7 Alexandre Poirot [:ochameau] 2013-04-15 09:03:46 PDT
Created attachment 737526 [details]
Pull request 942

Here is a patch to force using page zone for content script.
Comment 8 Alexandre Poirot [:ochameau] 2013-04-15 10:14:51 PDT
As for previous patch, I'm not sure this patch is mandatory?
Aren't we sharing zone compartment of the given principal?

We do something similar to this, with contentWindow being a reference to the page window object:
  Cu.Sandbox(contentWindow {sandboxPrototype: contentWindow, ...}) ?
Comment 9 Bobby Holley (PTO through June 13) 2013-04-15 12:40:18 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #8)
> As for previous patch, I'm not sure this patch is mandatory?
> Aren't we sharing zone compartment of the given principal?
> 
> We do something similar to this, with contentWindow being a reference to the
> page window object:
>   Cu.Sandbox(contentWindow {sandboxPrototype: contentWindow, ...}) ?

All sandboxes go into the system zone, regardless of their principal, unless you pass sameZoneAs.
Comment 10 [github robot] 2013-04-16 12:46:52 PDT
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/e7d0e6171f9f9be8cf29106ff182a1547f702e99
Bug 844180: Share content script and page zones.

https://github.com/mozilla/addon-sdk/commit/ab0413ef0f12c063a592cd1c2552cfdca77ad7d1
Merge pull request #942 from ochameau/contentScriptZone

Bug 844180: Share content script and page zones. r=@mossop
Comment 11 [github robot] 2013-04-17 11:55:50 PDT
Commits pushed to integration at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/e7d0e6171f9f9be8cf29106ff182a1547f702e99
Bug 844180: Share content script and page zones.

https://github.com/mozilla/addon-sdk/commit/ab0413ef0f12c063a592cd1c2552cfdca77ad7d1
Merge pull request #942 from ochameau/contentScriptZone
Comment 12 Dave Townsend [:mossop] 2013-05-22 14:20:08 PDT
This looks fixed to me.

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