800166 - Fire low-memory notifications when a process is backgrounded (and ensure that this triggers a GC)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

This is probably a one-line addition in dom/ipc/ProcessPriorityManager.cpp.

Comment 1

[Justin Lebar (not reading bugmail)]

This would be a decent first bug for someone to gain experience with our memory-profiling tools on the device.  (Writing the code isn't so interesting, but you also need to make sure it does what we want.)  But let me know if you're going to work on this, because I may snatch it up.

Comment 2

[Justin Lebar (not reading bugmail)]

Given how infrequently we appear to be gc'ing, this seems like a no-brainer.  It shouldn't even slow the device down much, given that we renice the process when it goes into the background.

Comment 3

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

Agreed.  We have a low-mem notification coming (at some point!), but it will often be too late.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

I thought I might have a crack at this.
A couple of things, though: is it better to use PokeGC or GarbageCollectNow? Also, I don't really know how to use the memory profiling tools (or even what they are). I saw your post about getting memory reports and using about:memory, but I wouldn't really know what to do with that data.
Finally, I don't actually have a development device, yet (I'm using the emulator, but it seems to be working okay, with a few hiccups).

Comment 5

[Andreas Gal :gal]

Comment on attachment 670987 [details] [diff] [review]
patch

I think you want GarbageCollectNow() here and actually I think we want to CC as well. gregor?

Comment 6

[Gregor Wagner [:gwagner]]

(In reply to Andreas Gal :gal from comment #5)
> Comment on attachment 670987 [details] [diff] [review]
> patch
> 
> I think you want GarbageCollectNow() here and actually I think we want to CC
> as well. gregor?

Yes GarbageCollectNow followed by a CC sounds better to me.
We are already in the 1 sec delay callback, we don't have to wait any longer to trigger the GC.
The CC will trigger a GC again if necessary.

Comment 7

[Gregor Wagner [:gwagner]]

With GarbageCollectNow you can even trigger a non-incremental, shrinking GC. That's what we want :)

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 670987 [details] [diff] [review]
patch

Yeah, just like this.

Call whatever GC function the JS peeps tell you to do.  :)

Also, four lines of context please.  If you're using git, git bz will do this for you.  https://github.com/jlebar/moz-git-tools  If you're using hg, you can set it in your ~/.hgrc as 

[diff]
git = 1
showfunc = 1
unified = 8

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v2

I think this is ready for a review.

Comment 10

[Gregor Wagner [:gwagner]]

Comment on attachment 671637 [details] [diff] [review]
patch v2

Thx!

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thanks for the quick review.
I'm not sure how best to test this, though. The automated tests aren't working very well on my local machine/emulator (either that or this change seriously breaks things). Are there b2g builds/tests on try?

Comment 12

[Justin Lebar (not reading bugmail)]

(In reply to David Keeler from comment #11)
> Are there b2g builds/tests on try?

Not really.

I'd just push this.  Maybe to be safe, you could build without this patch and observe that the automated tests don't work well without it too.

Comment 13

[Nicholas Nethercote [inactive]]

> Also, four lines of context please.
>
> unified = 8

Yeah, eight is preferred.

Comment 14

[Gregor Wagner [:gwagner]]

(In reply to Justin Lebar [:jlebar] from comment #12)
> (In reply to David Keeler from comment #11)
> > Are there b2g builds/tests on try?
> 
> Not really.
> 
> I'd just push this.  Maybe to be safe, you could build without this patch
> and observe that the automated tests don't work well without it too.

Did you test that putting an app in the background actually triggers a GC? You can just monitor the logcat output.

Comment 15

[Justin Lebar (not reading bugmail)]

Yeesh, I should have thought of this before now:

The right thing to do here is to fire a memory-pressure notification, perhaps three times, like we do in about:memory (i.e., call nsIMemoryReporterManager::MinimizeMemoryUsage).

We should make sure that this does a GC in B2G (good chance it doesn't).  But sending the notification will also clear other caches, such as

 * decoded images
 * bfcache

My fault for totally dropping the ball here; I can write the new patch.

Comment 16

[Justin Lebar (not reading bugmail)]

Looks like javascript.options.gc_on_memory_pressure is true, so all we should need to do is to fire these heap-minimize notifications.  But I'll give this a whirl.

Comment 17

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v3: Fire a low-mem notification

Comment 18

[Justin Lebar (not reading bugmail)]

I tested on the device that this triggers GCs in the appropriate process at the appropriate time.

Comment 19

[Justin Lebar (not reading bugmail)]

Comment on attachment 672461 [details] [diff] [review]
Patch, v3: Fire a low-mem notification

Erm, Gregor reviewed the last patch.

The GC is triggered by nsJSEnvironment's nsMemoryPressureObserver, whose Observe() method does exactly what David's patch did.

Comment 20

[Andreas Gal :gal]

I think worker threads run their own JS engine and need to be GC'ed explicitly. bent knows the details.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 672461 [details] [diff] [review]
Patch, v3: Fire a low-mem notification

(In reply to Andreas Gal :gal from comment #20)
> I think worker threads run their own JS engine and need to be GC'ed
> explicitly. bent knows the details.

The worker runtime service observes "memory-pressure" and forces a GC on all workers. This should be sufficient.

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 672461 [details] [diff] [review]
Patch, v3: Fire a low-mem notification

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

::: dom/ipc/ProcessPriorityManager.cpp
@@ +302,5 @@
> +  // We're in the background; dump as much memory as we can.
> +  nsCOMPtr<nsIMemoryReporterManager> mgr =
> +    do_GetService("@mozilla.org/memory-reporter-manager;1");
> +  if (mgr) {
> +    mgr->MinimizeMemoryUsage(/* callback = */ nullptr);

So you're doing the full three-times-around dance?  Interesting.

Comment 23

[Justin Lebar (not reading bugmail)]

> So you're doing the full three-times-around dance?  Interesting.

I figure it's worth trying.  In theory, this process was just reniced to 10, so in theory it shouldn't compete for CPU from another process that's actually doing something.

Comment 24

[Gregor Wagner [:gwagner]]

Comment on attachment 672461 [details] [diff] [review]
Patch, v3: Fire a low-mem notification

Looks good to me.

Comment 25

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56a682c947c4

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/56a682c947c4

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/979bb0c18664