For example, a WebappsApplication object holds a copy of the app's manifest. The manifest may include an icon encoded as a data URI. Since the icon can be of arbitrarily-large size, the manifest can be of arbitrarily-large size. But there's no reason to keep more than one copy of the icon data URI in memory. We should just intern this string (or maybe the whole manifest, or something like that; I don't really know how this works yet).

Per bug 889984 comment 5, Gaia leaks a lot of DOMApplication objects. This may happen only in Marionette, or it may happen all the time; I'm not sure yet. We can try to fix these leaks, but the more pressing matter, IMO, should be reducing the size of these leaked objects, so things don't break. So therefore this bug blocks bug 851626.

Wrapped manifests are cached on a per-window basis, but we don't cache the update manifests though, so doing that may help and is an easy fix. I think that gaia holds apps around both in the system app and in the homescreen, for valid reasons like listening to update events. I'll check how many objects are created in each app. The goal should be to have a single app per window for a given manifest URL.

> Wrapped manifests are cached on a per-window basis, Would it be possible to cache them on a per-process basis, or otherwise intern some of the strings in these objects? It's not clear to me yet whether this cache is somehow failing, or whether we simply have a bunch of windows. But given how big these objects can be, we need to be aggressive in sharing memory. > but we don't cache the update manifests though, so doing that may help and is an easy > fix. What's an update manifest? The things we were leaking looked like regular manifests, as far as I could tell, but that's only because I never saw the words "update manifest." :) Not to suggest that this wouldn't be useful.

(In reply to Justin Lebar [:jlebar] from comment #3) > > Wrapped manifests are cached on a per-window basis, > > Would it be possible to cache them on a per-process basis, or otherwise > intern some of the strings in these objects? It's not clear to me yet > whether this cache is somehow failing, or whether we simply have a bunch of > windows. But given how big these objects can be, we need to be aggressive > in sharing memory. The issue is that we have to wrap the manifests at the chrome -> content boundary, and this wrapping is per-window. I don't think we keep the unwrapped ones around but I can be wrong. > > but we don't cache the update manifests though, so doing that may help and is an easy > > fix. > > What's an update manifest? The things we were leaking looked like regular > manifests, as far as I could tell, but that's only because I never saw the > words "update manifest." :) Not to suggest that this wouldn't be useful. The update manifests are the small ones that are used for the initial install of packaged apps and for updates. They are smaller usually, but could contain icons indeed.

Like I say, caching per-window is likely leading to unacceptable memory performance under our current workload, and it's trivial to create a different workload under which it definitely would be unacceptable. Surely we have some options here?

This patch does a couple of different things: 1. it fixes a leak where we kept unwrapped manifests around needlessly. 2. it introduces the possibility for ObjectWrapper to intern strings. Here are some results I got, with a build that only has the default apps (so no huge data: uri for the icons) : Explicit memory of (Main, Homescreen, Usage, Preallocated, total) No patch : 38.36 7.57 6.85 5.27 58.05 Applying string interning only to manifest objects, with different values for the threshold that used to control which strings we intern: manifest 0 : 36.21 11.68 6.77 5.27 59.93 manifest 4 : 36.87 7.52 6.78 5.27 56.44 manifest 8 : 36.55 7.58 6.81 5.27 56.21 manifest 16: 36.62 7.50 6.81 5.27 56.20 manifest 32: 36.25 7.49 6.79 5.27 55.80 And when interning strings for all wrapped objects: all 0 : 36.22 11.69 6.77 5.27 59.95 all 4 : 36.26 8.69 6.79 5.27 57.01 all 8 : 36.17 10.23 6.77 5.27 58.44 all 16: 36.77 7.51 6.78 5.27 56.33 all 32: 36.18 10.19 6.80 5.27 58.44 "Manifest 16" looks like the best case, but I feel we need to measure more, and also not just with the default apps. Justin, let me know which config you used (eg. the spanish customization?) and if the numbers above are a worthwhile gain.

It's a bummer that Gecko/SM doesn't give you an API that says "intern this string" and that you have to instead do it yourself. :-/ I have no idea which config I was using, but in any case, what's most important is what our partners have been using to test; atm they can't run any marionette endurance tests because of this issue, e.g. bug 851626, bug 886217.

(In reply to Justin Lebar [:jlebar] from comment #7) > It's a bummer that Gecko/SM doesn't give you an API that says "intern this > string" and that you have to instead do it yourself. :-/ > SM can do it :) https://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#5925

Core Graveyard / DOM: Apps is inactive. Closing all bugs in this component.