Last Comment Bug 680821 - Update Add-on sandbox to use new memory accounting options
: Update Add-on sandbox to use new memory accounting options
Status: RESOLVED FIXED
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: 1.2
Assigned To: Sander Mathijs van Veen
:
Mentors:
: 680958 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-21 19:37 PDT by Josh Matthews [:jdm]
Modified: 2012-01-10 14:47 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Report sandbox filenames in about:memory (558 bytes, patch)
2011-08-21 22:39 PDT, Wes Kocher (:KWierso)
myk: review+
Details | Diff | Review
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323 (355 bytes, text/html)
2012-01-09 10:27 PST, Irakli Gozalishvili [:irakli] [:gozala] [@gozala]
no flags Details
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323# (358 bytes, text/html)
2012-01-09 10:28 PST, Irakli Gozalishvili [:irakli] [:gozala] [@gozala]
no flags Details
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323# (358 bytes, text/html)
2012-01-09 10:28 PST, Irakli Gozalishvili [:irakli] [:gozala] [@gozala]
no flags Details
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323# (358 bytes, text/html)
2012-01-09 10:28 PST, Irakli Gozalishvili [:irakli] [:gozala] [@gozala]
no flags Details
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323# (358 bytes, text/html)
2012-01-09 10:28 PST, Irakli Gozalishvili [:irakli] [:gozala] [@gozala]
no flags Details

Description Josh Matthews [:jdm] 2011-08-21 19:37:51 PDT
The patch is already written here: https://bug673331.bugzilla.mozilla.org/attachment.cgi?id=550570
Comment 1 Byron Jones ‹:glob› 2011-08-21 22:11:07 PDT
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.
Comment 2 Nicholas Nethercote [:njn] 2011-08-21 22:20:22 PDT
(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.
Comment 3 Wes Kocher (:KWierso) 2011-08-21 22:39:10 PDT
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
Comment 4 Wes Kocher (:KWierso) 2011-08-21 22:40:46 PDT
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.
Comment 5 Byron Jones ‹:glob› 2011-08-22 10:47:25 PDT
*** Bug 680958 has been marked as a duplicate of this bug. ***
Comment 6 Gabor Krizsanits [:krizsa :gabor] 2011-08-24 08:57:12 PDT
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.
Comment 7 Brian Warner [:warner :bwarner] 2011-08-30 16:27:54 PDT
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.
Comment 8 Myk Melez [:myk] [@mykmelez] 2011-09-01 16:56:45 PDT
(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?
Comment 9 Brian Warner [:warner :bwarner] 2011-09-01 17:41:04 PDT
sounds good to me!
Comment 10 Nicholas Nethercote [:njn] 2011-09-01 17:50:26 PDT
(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.
Comment 11 Gabor Krizsanits [:krizsa :gabor] 2011-09-02 01:10:02 PDT
(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 12 Myk Melez [:myk] [@mykmelez] 2011-09-02 10:43:35 PDT
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.
Comment 13 Myk Melez [:myk] [@mykmelez] 2011-09-02 10:44:41 PDT
(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
Comment 15 Nickolay_Ponomarev 2012-01-04 07:30:06 PST
This change seems to have been undone in the new loader:
 $ grep -r "sandboxName" * | wc -l
       0

Or was there another reason for that?
Comment 16 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2012-01-09 10:27:58 PST
Created attachment 587035 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323

Pointer to Github pull-request
Comment 17 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2012-01-09 10:28:22 PST
Created attachment 587037 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Pointer to Github pull-request
Comment 18 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2012-01-09 10:28:22 PST
Created attachment 587038 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Pointer to Github pull-request
Comment 19 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2012-01-09 10:28:36 PST
Created attachment 587039 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Pointer to Github pull-request
Comment 20 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2012-01-09 10:28:53 PST
Created attachment 587040 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Pointer to Github pull-request
Comment 21 Nicholas Nethercote [:njn] 2012-01-09 15:37:19 PST
I don't understand how the add-on team uses Github.  Is this bug fixed?  If not, what remains to be done?
Comment 22 Wes Kocher (:KWierso) 2012-01-09 15:54:31 PST
(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 23 Alexandre Poirot [:ochameau] 2012-01-10 07:09:37 PST
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.
Comment 24 Alexandre Poirot [:ochameau] 2012-01-10 14:43:38 PST
Fixed by recent landing in bug 587197
Comment 25 Myk Melez [:myk] [@mykmelez] 2012-01-10 14:47:02 PST
(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.

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