889990 - WebappsApplication objects should share memory

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[Justin Lebar (not reading bugmail)]

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).

Comment 1

[Justin Lebar (not reading bugmail)]

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.

Comment 2

[[:fabrice] Fabrice Desré]

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.

Comment 3

[Justin Lebar (not reading bugmail)]

> 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.

Comment 4

[[:fabrice] Fabrice Desré]

(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.

Comment 5

[Justin Lebar (not reading bugmail)]

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?

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: wip

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.

Comment 7

[Justin Lebar (not reading bugmail)]

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.

Comment 8

[Gregor Wagner [:gwagner]]

(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

Comment 9

[Sylvestre Ledru [:Sylvestre]]

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