Last Comment Bug 800166 - Fire low-memory notifications when a process is backgrounded (and ensure that this triggers a GC)
: Fire low-memory notifications when a process is backgrounded (and ensure that...
Status: RESOLVED FIXED
[MemShrink:P1]
:
Product: Firefox OS
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 768832
Blocks: slim-fast
  Show dependency treegraph
 
Reported: 2012-10-10 15:36 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-10-24 09:13 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
patch (622 bytes, patch)
2012-10-12 16:52 PDT, David Keeler [:keeler] (use needinfo?)
justin.lebar+bug: feedback+
Details | Diff | Splinter Review
patch v2 (1.55 KB, patch)
2012-10-15 15:54 PDT, David Keeler [:keeler] (use needinfo?)
anygregor: review+
Details | Diff | Splinter Review
Patch, v3: Fire a low-mem notification (1.88 KB, patch)
2012-10-17 12:38 PDT, Justin Lebar (not reading bugmail)
anygregor: review+
bent.mozilla: feedback+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-10-10 15:36:23 PDT
This is probably a one-line addition in dom/ipc/ProcessPriorityManager.cpp.
Comment 1 Justin Lebar (not reading bugmail) 2012-10-10 16:16:09 PDT
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) 2012-10-11 21:24:40 PDT
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 2012-10-11 21:26:48 PDT
Agreed.  We have a low-mem notification coming (at some point!), but it will often be too late.
Comment 4 David Keeler [:keeler] (use needinfo?) 2012-10-12 16:52:50 PDT
Created attachment 670987 [details] [diff] [review]
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 2012-10-12 17:20:39 PDT
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] 2012-10-12 18:48:24 PDT
(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] 2012-10-12 19:28:47 PDT
With GarbageCollectNow you can even trigger a non-incremental, shrinking GC. That's what we want :)
Comment 8 Justin Lebar (not reading bugmail) 2012-10-13 07:03:41 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-10-15 15:54:17 PDT
Created attachment 671637 [details] [diff] [review]
patch v2

I think this is ready for a review.
Comment 10 Gregor Wagner [:gwagner] 2012-10-15 19:55:36 PDT
Comment on attachment 671637 [details] [diff] [review]
patch v2

Thx!
Comment 11 David Keeler [:keeler] (use needinfo?) 2012-10-16 16:53:36 PDT
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) 2012-10-16 17:12:26 PDT
(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 [:njn] 2012-10-16 17:23:38 PDT
> Also, four lines of context please.
>
> unified = 8

Yeah, eight is preferred.
Comment 14 Gregor Wagner [:gwagner] 2012-10-16 17:26:59 PDT
(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) 2012-10-17 11:34:30 PDT
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) 2012-10-17 11:43:00 PDT
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) 2012-10-17 12:38:48 PDT
Created attachment 672461 [details] [diff] [review]
Patch, v3: Fire a low-mem notification
Comment 18 Justin Lebar (not reading bugmail) 2012-10-17 12:39:17 PDT
I tested on the device that this triggers GCs in the appropriate process at the appropriate time.
Comment 19 Justin Lebar (not reading bugmail) 2012-10-17 12:41:53 PDT
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 2012-10-17 13:08:59 PDT
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!) 2012-10-17 14:16:57 PDT
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 [:njn] 2012-10-17 15:23:03 PDT
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) 2012-10-17 15:26:56 PDT
> 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] 2012-10-22 12:37:22 PDT
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) 2012-10-22 12:41:15 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/56a682c947c4
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-10-22 19:00:44 PDT
https://hg.mozilla.org/mozilla-central/rev/56a682c947c4
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-10-22 20:28:13 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/979bb0c18664

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