797203 - Keyboard's windows and IME worker continue to use memory even after keyboard is closed.

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The keyboard will very frequently be open.  It will run in-process for v1, but it's memory usage needs to be absolutely as small as possible, because its memory is denied from app subprocesses.

Vivien tells me it uses something like 10MB (!!!) currently..

Comment 1

[Justin Lebar (not reading bugmail)]

Andrea, this might be a bug for you to dig into.  The keyboard runs in the main process, so it would be particularly bad if we didn't release all of the keyboard's memory when we close it.

Comment 2

[Andreas Gal :gal]

A big amount of memory for the keyboard is the predictive text library (5+ MB typed array). The keyboard could release that on memory pressure or on a timer (30s no activity etc). Downside is that it has to load it again, but that might be worth it.

Comment 3

[Justin Lebar (not reading bugmail)]

(In reply to Andreas Gal :gal from comment #2)
> A big amount of memory for the keyboard is the predictive text library (5+
> MB typed array). The keyboard could release that on memory pressure or on a
> timer (30s no activity etc). Downside is that it has to load it again, but
> that might be worth it.

Do I have to do something special to enable predictive test (or, did it just land a day ago or something)?  I don't see predictive text or the large compartment in a build I did a few days ago:

│   │  ├──0.98 MB (02.85%) -- window(app://keyboard.gaiamobile.org/index.html#show=198)
│   │  │  ├──0.66 MB (01.90%) -- js/compartment(app://keyboard.gaiamobile.org/index.html)
│   │  │  │  ├──0.41 MB (01.18%) ++ gc-heap
│   │  │  │  └──0.25 MB (00.72%) ++ (6 tiny)
│   │  │  └──0.33 MB (00.95%) ++ (4 tiny)

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

People keep claiming it's on by default, but I've never seen it.  Can enable in Settings.

Comment 5

[Justin Lebar (not reading bugmail)]

Okay...with predictive text on, I see

│   │  ├──1.49 MB (03.09%) -- window(app://keyboard.gaiamobile.org/index.html#show=228)
│   │  │  ├──1.10 MB (02.29%) -- js/compartment(app://keyboard.gaiamobile.org/index.html)
│   │  │  │  ├──0.42 MB (00.87%) ── analysis-temporary
│   │  │  │  ├──0.41 MB (00.85%) -- gc-heap
│   │  │  │  │  ├──0.19 MB (00.41%) ++ objects
│   │  │  │  │  ├──0.10 MB (00.21%) ++ shapes
│   │  │  │  │  ├──0.05 MB (00.10%) ── unused-gc-things
│   │  │  │  │  ├──0.03 MB (00.07%) ── strings/normal
│   │  │  │  │  ├──0.01 MB (00.03%) ── scripts
│   │  │  │  │  ├──0.01 MB (00.02%) ── sundries
│   │  │  │  │  └──0.01 MB (00.02%) ── type-objects
│   │  │  │  ├──0.05 MB (00.10%) ── jaeger-data
│   │  │  │  ├──0.04 MB (00.09%) ── script-data
│   │  │  │  ├──0.04 MB (00.09%) ++ shapes-extra
│   │  │  │  ├──0.04 MB (00.09%) ++ type-inference
│   │  │  │  ├──0.03 MB (00.07%) ── other-sundries
│   │  │  │  ├──0.03 MB (00.06%) ── string-chars/huge/string(length=5751, "<div class="keyboard-row first-...") [2]
│   │  │  │  └──0.03 MB (00.06%) ── objects-extra/slots
│   │  │  └──0.38 MB (00.80%) -- (4 tiny)
│   │  │     ├──0.25 MB (00.51%) ++ layout
│   │  │     ├──0.08 MB (00.17%) ++ dom
│   │  │     ├──0.06 MB (00.12%) ── style-sheets
│   │  │     └──0.00 MB (00.00%) ── property-tables

but maybe the dictionary doesn't live in the keyboard compartment for some reason.

Comment 6

[Justin Lebar (not reading bugmail)]

Ah, here it is.  (It was under "IME", not "keyboard".)

├───6.19 MB (13.29%) -- workers
│   ├──6.19 MB (13.29%) -- workers(gaiamobile.org)/worker(js/imes/latin/worker.js, 49ed3c00)
│   │  ├──3.74 MB (08.02%) -- compartment(web-worker)
│   │  │  ├──3.47 MB (07.44%) -- objects-extra
│   │  │  │  ├──3.45 MB (07.40%) ── elements
│   │  │  │  └──0.02 MB (00.04%) ── slots
│   │  │  └──0.27 MB (00.58%) ++ (6 tiny)
│   │  ├──1.72 MB (03.70%) -- gc-heap
│   │  │  ├──1.66 MB (03.56%) ── decommitted-arenas
│   │  │  └──0.06 MB (00.13%) ++ (3 tiny)
│   │  ├──0.63 MB (01.35%) ++ runtime
│   │  └──0.10 MB (00.21%) ++ compartment(web-worker-atoms)
│   └──0.00 MB (00.00%) ++ workers()

Comment 7

[Justin Lebar (not reading bugmail)]

As far as I can tell, this worker never goes away.

Comment 8

[Justin Lebar (not reading bugmail)]

How long does it take to spin up the predictive text engine?  Can we drop it after the keyboard hasn't been shown for X seconds?

We can also drop it on memory pressure, but memory pressure events are so fiddly, I think it's safer if that's a last resort, rather than the primary method of reclaiming this memory.

Comment 9

[Andreas Gal :gal]

We have to read 6mb-ish off the disk, or from the cache, if we are lucky. I think its worth spinning down the worker. It starts async so the keyboard is responsive we just won't get suggestions for a few seconds (probably low number, 2-3s?), even worst case. Lets spin it down on a timer.

Comment 10

[Andreas Gal :gal]

We are implementing this based on a 30s timer. If we don't release the memory, this is a bug. Note that we don't GC, just release the memory. See idle() in apps/keyboard/js/imes/latin/worker.js

Comment 11

[Justin Lebar (not reading bugmail)]

Okay, I see us dropping the data blob after waiting for ~30s.

But I still see us using a fair bit of memory for the keyboard.  These numbers are after closing the keyboard, waiting 30s, and minimizing memory usage (assuming that works properly).

I also think we should try to drop the blob on memory pressure, but that's a lower priority.

│   ├──2.74 MB (07.00%) -- workers(gaiamobile.org)/worker(js/imes/latin/worker.js, 49ed3c00)
│   │  ├──1.72 MB (04.40%) -- gc-heap
│   │  │  ├──1.69 MB (04.32%) ── decommitted-arenas
│   │  │  └──0.03 MB (00.08%) -- (3 tiny)
│   │  │     ├──0.03 MB (00.08%) ── chunk-admin
│   │  │     ├──0.00 MB (00.00%) ── unused-arenas
│   │  │     └──0.00 MB (00.00%) ── unused-chunks
│   │  ├──0.63 MB (01.61%) ++ runtime
│   │  └──0.39 MB (00.99%) -- (2 tiny)
│   │     ├──0.29 MB (00.74%) ++ compartment(web-worker)
│   │     └──0.10 MB (00.25%) ++ compartment(web-worker-atoms)

You can subtract 1.69mb here; decommitted memory is basically free.

├──1.35 MB (16.73%) -- window-objects/top(app://settings.gaiamobile.org/index.html#keyboard, id=1)/active/window(app://settings.gaiamobile.org/index.html#keyboard)
│  ├──0.55 MB (06.76%) -- js/compartment(app://settings.gaiamobile.org/index.html#root)
│  │  ├──0.33 MB (04.07%) -- gc-heap
│  │  │  ├──0.10 MB (01.28%) ── unused-gc-things
│  │  │  ├──0.09 MB (01.16%) ++ shapes
│  │  │  ├──0.09 MB (01.09%) ++ objects
│  │  │  └──0.04 MB (00.54%) ++ (3 tiny)
│  │  └──0.22 MB (02.70%) -- (6 tiny)
│  │     ├──0.07 MB (00.90%) ++ string-chars
│  │     ├──0.05 MB (00.62%) ── script-data
│  │     ├──0.04 MB (00.43%) ++ shapes-extra
│  │     ├──0.03 MB (00.36%) ── analysis-temporary
│  │     ├──0.02 MB (00.27%) ── objects-extra/slots
│  │     └──0.01 MB (00.12%) ── other-sundries

├──0.99 MB (13.65%) -- window-objects/top(app://uitest.gaiamobile.org/index.html#test=keyboard, id=1)/active
│  ├──0.57 MB (07.88%) -- window(app://uitest.gaiamobile.org/index.html#test=keyboard)
│  │  ├──0.28 MB (03.92%) -- layout
│  │  │  ├──0.14 MB (01.87%) ── pres-shell
│  │  │  ├──0.10 MB (01.39%) ── style-sets
│  │  │  └──0.05 MB (00.66%) ++ (5 tiny)
│  │  ├──0.25 MB (03.51%) -- js/compartment(app://uitest.gaiamobile.org/index.html)
│  │  │  ├──0.18 MB (02.42%) ++ gc-heap
│  │  │  └──0.08 MB (01.09%) ++ (4 tiny)
│  │  └──0.03 MB (00.46%) ++ (3 tiny)

So the total is ~3mb in the main process that we don't seem to release.  But the problem isn't the IME's typed array, which we do seem to get rid of.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: Gaia patch

I'm going to terminate the worker instead release the dictionary.
This is the performance difference between the 2 approaches:

BAKU START is written when the worker is required and before sending 'setLanguage'
BAKU READY is written by the worker when the dictionary is loaded.

Terminating the worker, the worker is null and we have:

11-09 12:24:03.161  3261  3261 I GeckoDump: BAKU START => null
11-09 12:24:05.193  3261  3356 I Gecko   : BAKU READY!

~ 2secs.

Again:
11-09 12:24:20.318  3261  3261 I GeckoDump: BAKU START => null
11-09 12:24:22.370  3261  3356 I Gecko   : BAKU READY!

~ 2secs.

Here the same measurement, just dropping the dictionary:
 
11-09 12:27:34.007  3383  3383 I GeckoDump: BAKU START => [object Worker]
11-09 12:27:36.019  3383  3478 I Gecko   : BAKU READY!

~ 2secs.

again:
11-09 12:27:52.835  3383  3383 I GeckoDump: BAKU START => [object Worker]
11-09 12:27:54.827  3383  3478 I Gecko   : BAKU READY!

~ 2secs.

Comment 13

[Justin Lebar (not reading bugmail)]

Comment on attachment 680195 [details] [diff] [review]
Gaia patch

This sounds good to me (it gains us 1-2mb of memory in the main process), but I'm not a Gaia reviewer.  Can you look through the git log and find out who owns this code?

Note that we're not entirely done here; I think we should also release the keyboard window.  But we can do that separately.

Comment 14

[David Flanagan [:djf]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> People keep claiming it's on by default, but I've never seen it.  Can enable
> in Settings.

Just FYI: It just went on by default yesterday. You have to do a reset-gaia to push settings.json to the phone and have the phone rebuild the settings db to get the new defaults.

Comment 15

[David Flanagan [:djf]]

Comment on attachment 680195 [details] [diff] [review]
Gaia patch

Review of attachment 680195 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good to me. I haven't attempted to verify that it actually saves more memory than the old way.  But this approach of killing the worker is a lot simpler than what I was doing before to drop the dictionary.

Comment 16

[Andrea Marchesini [:baku]]

https://github.com/mozilla-b2g/gaia/pull/6331
pull request sent.

Comment 17

[Andreas Gal :gal]

This is some fine work guys. Thanks!

Comment 18

[Andreas Gal :gal]

a=gal, merged

Comment 19

[Justin Lebar (not reading bugmail)]

> Note that we're not entirely done here; I think we should also release the keyboard 
> window.

Andrea, can you file a bug on this?  Or, if you have filed the bug, can you mark it blocking slim-fast.  (You don't need to commit to fixing the bug if you don't want to, but I don't want to lose track of the issue...)

Comment 20

[Andrea Marchesini [:baku]]

Bug 810686