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

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.

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.

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

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 on attachment 670987 [details] [diff] [review] patch I think you want GarbageCollectNow() here and actually I think we want to CC as well. gregor?

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

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

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

I think this is ready for a review.

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

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?

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

> Also, four lines of context please. > > unified = 8 Yeah, eight is preferred.

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

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.

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.

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

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.

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

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

> 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 on attachment 672461 [details] [diff] [review] Patch, v3: Fire a low-mem notification Looks good to me.