The default bug view has changed. See this FAQ.

Update Add-on sandbox to use new memory accounting options

RESOLVED FIXED in 1.2

Status

Add-on SDK
General
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jdm, Assigned: Sander Mathijs van Veen)

Tracking

unspecified

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
The patch is already written here: https://bug673331.bugzilla.mozilla.org/attachment.cgi?id=550570
the patched file is part of the add-on sdk, not part of tweaks, so that's where this bug belongs; moving.

i've applied the patch to my sdk environment, so any build of tweaks that i produce will include this change.
Component: Bugzilla Tweaks → General
Product: bugzilla.mozilla.org → Add-on SDK
QA Contact: tweaks → general
Version: Current → unspecified
Summary: Update bugzilla tweaks' sandbox to use new memory accounting options → Update Add-on sandbox to use new memory accounting options
(In reply to Byron Jones ‹:glob› from comment #1)
> the patched file is part of the add-on sdk, not part of tweaks, so that's
> where this bug belongs; moving.

That's good news, because it means many add-ons will benefit from this change, not just one.
Created attachment 554795 [details] [diff] [review]
Report sandbox filenames in about:memory

[23:53]	<glob>	i have a bugzilla tweaks xpi with the patch from bug 680821 which may help; http://dl.dropbox.com/u/16292140/bugzilla-tweaks-1.11b.xpi

[00:06]	<glob>	njn, with regards to the sandboxName property and tweaks; the file that needs to be updated is part of the sdk, not tweaks itself, so really that's where the fix belongs
[00:07]	<njn>	glob: ok, so maybe the componet of bug 680821 should be changed?
	<glob>	njn, doing it now
	<njn>	glob: thx
[00:08]	<glob>	njn, i'm not sure if the patch is generic enough for all add-ons, i trust the jetpack guys will do the right thing
	<njn>	glob: if they don't know, god help us all
	<glob>	:)

[00:20]	<KWierso>	njn: does that jetpack bug need to be done soonish rather than laterish?
[00:21]	<njn>	KWierso: it would be nice if it were done by the next add-on SDK build, wheenver that is
	<KWierso>	is that patch safe for all current firefox versions?
[00:24]	njn: ^, or does that only work for Nightly builds?
	<njn>	KWierso: I think it's ineffective but harmless for non-Nightly builds, but I'm not 100% certain
	<glob>	KWierso, it works in aurora
[00:25]	<njn>	glob: how?
	<glob>	where "works" = doesn't cause an end of the world event
Myk, who would review this? 
The patch I attached is the same as the one linked in comment 0, just based off the SDK github master.
Duplicate of this bug: 680958

Updated

6 years ago
Attachment #554795 - Flags: review?(myk)
Filename? Shouldn't that be an addon name? With file names it will be bloated imo, it's much more interesting how much memory an addon uses than the module files it's using (since then you won't know which addon is loaded that module file) Also, I already planned to use that property to identify addons (see Bug 677294 ) which I can change ofc, just I'm not sure that this is what we want.
Priority: -- → P2
Target Milestone: --- → 1.2
Incidentally, when the loader says options.filename, it probably really contains the URI (something like resource://$JETPACKID-$PACKAGE-$SECTION/main.js). The on-disk filename (starting with /home/warner/.firefox/profiles/etc) only shows up in the target of the resProt mapping.

If the memory-tracking code makes it easy to display a hierarchy, then it'd be useful to see both the per-module and the per-addon memory usage. If it's not, then I agree that sticking to just the addon name will make the output easier to use.
(In reply to Brian Warner [:warner :bwarner] from comment #7)
> If the memory-tracking code makes it easy to display a hierarchy, then it'd
> be useful to see both the per-module and the per-addon memory usage.

The memory-tracking code does display a hierarchy, but it doesn't appear to be configurable in this way.


> If it's
> not, then I agree that sticking to just the addon name will make the output
> easier to use.

I agree; however, the memory-tracking code doesn't particularly accommodate this either, since it doesn't coalesce statistics for the compartments of sandboxes with identical names; it reports each separately.


So right now there isn't a way to configure about:memory to show the total amount of memory being used by an addon, which is the most useful information for casual readers.  Nevertheless, folks want stats, and per-module stats are presumably better than nothing, so I suggest we do the following:

1. land the patch in this bug, to give folks the stats we can currently give them;
2. fix bug 672443 once platform blocker bug 677294 is fixed, so all modules load in a single compartment;
3. make improvements to about:memory as appropriate.

Regarding step #3, note that addons load web pages--in widgets, panels, tabs, etc.--in addition to modules, and those will continue to have their own compartments even after we load all modules into a single compartment, so the improvement to about:memory I'm particularly interested in is coalescing stats for multiple compartments associated with the same addon (which'll mean figuring out how to identify content frames as being associated with an addon).

Brian: sound reasonable?
sounds good to me!
(In reply to Myk Melez [:myk] from comment #8)
> 
> I agree; however, the memory-tracking code doesn't particularly accommodate
> this either, since it doesn't coalesce statistics for the compartments of
> sandboxes with identical names; it reports each separately.

about:memory does coalesce multiple reporters that have the same path, and puts a number in square brackets to indicate how many there were.  E.g. in the following example there were 4 storage/sqlite/places.sqlite/cache-used reporters:

├───25,561,968 B (06.52%) -- storage
│   └──25,561,968 B (06.52%) -- sqlite
│      ├──21,040,048 B (05.36%) -- places.sqlite
│      │  ├──20,670,880 B (05.27%) -- cache-used [4]
│      │  ├─────291,488 B (00.07%) -- stmt-used [4]
│      │  └──────77,680 B (00.02%) -- schema-used [4]

For system compartments, bug 672439 deliberately added the compartment's address to to the path used for each system compartment's reporter, so that they weren't coalesced.

It wouldn't be hard to only add the address to compartments that lack a name.  Then all compartments with the same name would be coalesced.
(In reply to Myk Melez [:myk] from comment #8)
> 2. fix bug 672443 once platform blocker bug 677294 is fixed, so all modules
> load in a single compartment;

Just one little correction for this. My (current) patch for bug 677294 will not put every module per add-on into the same compartment, only the ones that share the same principal. I do not think it would be a good idea to merge modules with system principals, with low privileged modules into the same compartment (I'm not even sure if it's possible).

Another thought that I will use another identifier string there probably (it is addonId now) so maybe that information can be used here as well. Basically each sandbox will 'know' which add-on they are belong to not just the path of it's module file.
Comment on attachment 554795 [details] [diff] [review]
Report sandbox filenames in about:memory

Trivial style nit: add space between the curly braces and their enclosed property declaration.
Attachment #554795 - Flags: review?(myk) → review+
(In reply to Nicholas Nethercote [:njn] from comment #10)
> It wouldn't be hard to only add the address to compartments that lack a
> name.  Then all compartments with the same name would be coalesced.

Ah, useful info!  To my mind, the ideal view would actually use hierarchy but not coalescence to reveal both total addon memory usage and usage by individual modules and content frames, so something like this:

├───### B (##.##%) -- addon foo@example.com
│   │──### B (##.##%) -- module addon-kit/panel
│   │──### B (##.##%) -- module api-utils/content/worker
│   │──### B (##.##%) -- frame http://www.example.com/

(Partial compartment sharing between modules may complicate the picture.  Also, I don't yet know how to identify frames as belonging to addons.)


(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)
> Just one little correction for this. My (current) patch for bug 677294 will
> not put every module per add-on into the same compartment, only the ones
> that share the same principal. I do not think it would be a good idea to
> merge modules with system principals, with low privileged modules into the
> same compartment (I'm not even sure if it's possible).

For the purposes of about:memory reporting, that seems fine, as long as it is possible to identify both the privileged and the non-privileged compartments as belonging to the same addon.


> Another thought that I will use another identifier string there probably (it
> is addonId now) so maybe that information can be used here as well.
> Basically each sandbox will 'know' which add-on they are belong to not just
> the path of it's module file.

Yup, definitely.  Once it is possible to identify sandboxes not only by their individual identities (which in this case correspond to modules) but also by their membership in a group (which in this case is an addon), it should be possible to reflect that information usefully into about:memory.


Landed, and credited to Sander, who submitted the original patch over in bug 673331:

https://github.com/mozilla/addon-sdk/commit/544ab803ef894978a9d31a4418a5bd6397cc070e
Assignee: nobody → sandervv
OS: Mac OS X → All
Hardware: x86 → All
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
https://github.com/mozilla/addon-sdk/commit/39830898891b13a2e4893dd49ecc76223b9ce02d

Comment 15

5 years ago
This change seems to have been undone in the new loader:
 $ grep -r "sandboxName" * | wc -l
       0

Or was there another reason for that?
Created attachment 587035 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323

Pointer to Github pull-request
Created attachment 587037 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Pointer to Github pull-request
Created attachment 587038 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Pointer to Github pull-request
Created attachment 587039 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Pointer to Github pull-request
Created attachment 587040 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Pointer to Github pull-request
Attachment #587040 - Attachment is obsolete: true
Attachment #587035 - Attachment is obsolete: true
Attachment #587037 - Flags: review?(poirot.alex)
Attachment #587038 - Attachment is obsolete: true
Attachment #587039 - Attachment is obsolete: true
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I don't understand how the add-on team uses Github.  Is this bug fixed?  If not, what remains to be done?
(In reply to Nicholas Nethercote [:njn] from comment #21)
> I don't understand how the add-on team uses Github.  Is this bug fixed?  If
> not, what remains to be done?

It was fixed, then a recent change in the SDK's module loader undid the fix, and pull request 323 will put the fix back in.
Comment on attachment 587037 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Another patch has been submitted by myk in bug 587197 that will fix this and at the same time use `sameGroupAs` attribute in order to share compartments.
So I prefer to land his patch as it will conflict with this one.
Attachment #587037 - Flags: review?(poirot.alex)
Fixed by recent landing in bug 587197
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
(In reply to Alexandre Poirot (:ochameau) from comment #24)
> Fixed by recent landing in bug 587197

Erm, actually it was the landing in bug 672443 that resolves this issue.
You need to log in before you can comment on or make changes to this bug.