The default bug view has changed. See this FAQ.

Share zones across SDK modules

RESOLVED FIXED

Status

Add-on SDK
General
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mossop, Assigned: ochameau)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.

Updated

4 years ago
OS: Windows 8 → All
Priority: -- → P2
Hardware: x86_64 → All
(Assignee)

Comment 1

4 years ago
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!
Attachment #725895 - Flags: review?(rFobic)
Whiteboard: [MemShrink]
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?
Whiteboard: [MemShrink]
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #725895 - Attachment is obsolete: true
Attachment #725895 - Flags: review?(rFobic)
Whiteboard: [MemShrink]
(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).
Whiteboard: [MemShrink] → [MemShrink:P3]
(Assignee)

Comment 5

4 years ago
(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...
(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.
(Assignee)

Comment 7

4 years ago
Created attachment 737526 [details]
Pull request 942

Here is a patch to force using page zone for content script.
Attachment #737526 - Flags: review?(dtownsend+bugmail)
(Assignee)

Comment 8

4 years ago
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, ...}) ?
(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.
(Reporter)

Updated

4 years ago
Attachment #737526 - Flags: review?(dtownsend+bugmail) → review+

Comment 10

4 years ago
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

4 years ago
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
(Reporter)

Comment 12

4 years ago
This looks fixed to me.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.