Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug)

unspecified
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(1 attachment, 2 obsolete attachments)

1.88 KB, patch
gwagner
: review+
Ben Turner (not reading bugmail, use the needinfo flag!)
: feedback+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
This is probably a one-line addition in dom/ipc/ProcessPriorityManager.cpp.
(Assignee)

Comment 1

5 years ago
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.
Whiteboard: [MemShrink] → [MemShrink][mentor=jlebar][lang=c++]
(Assignee)

Comment 2

5 years ago
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.
blocking-basecamp: --- → ?
Agreed.  We have a low-mem notification coming (at some point!), but it will often be too late.
blocking-basecamp: ? → +
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).
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #670987 - Flags: feedback?(justin.lebar+bug)

Comment 5

5 years ago
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?
Attachment #670987 - Flags: feedback?(anygregor)
(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 :)
(Assignee)

Comment 8

5 years ago
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
Attachment #670987 - Flags: feedback?(justin.lebar+bug) → feedback+
Created attachment 671637 [details] [diff] [review]
patch v2

I think this is ready for a review.
Attachment #670987 - Attachment is obsolete: true
Attachment #670987 - Flags: feedback?(anygregor)
Attachment #671637 - Flags: review?(anygregor)
Comment on attachment 671637 [details] [diff] [review]
patch v2

Thx!
Attachment #671637 - Flags: review?(anygregor) → review+
Whiteboard: [MemShrink][mentor=jlebar][lang=c++] → [MemShrink:P1][mentor=jlebar][lang=c++]
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?
Whiteboard: [MemShrink:P1][mentor=jlebar][lang=c++] → [MemShrink][mentor=jlebar][lang=c++]
(Assignee)

Comment 12

5 years ago
(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.
Whiteboard: [MemShrink][mentor=jlebar][lang=c++] → [MemShrink:P1][mentor=jlebar][lang=c++]
(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.
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: dkeeler → justin.lebar+bug
(Assignee)

Updated

5 years ago
Summary: GC a process when it's backgrounded → Fire low-memory notifications when a process is backgrounded (and ensure that this triggers a GC)
(Assignee)

Updated

5 years ago
Whiteboard: [MemShrink:P1][mentor=jlebar][lang=c++] → [MemShrink:P1]
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Created attachment 672461 [details] [diff] [review]
Patch, v3: Fire a low-mem notification
Attachment #671637 - Attachment is obsolete: true
Attachment #672461 - Flags: review?(wmccloskey)
(Assignee)

Comment 18

5 years ago
I tested on the device that this triggers GCs in the appropriate process at the appropriate time.
(Assignee)

Comment 19

5 years ago
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.
Attachment #672461 - Flags: review?(wmccloskey) → review?(anygregor)

Comment 20

5 years ago
I think worker threads run their own JS engine and need to be GC'ed explicitly. bent knows the details.
(Assignee)

Updated

5 years ago
Attachment #672461 - Flags: feedback?(bent.mozilla)
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.
Attachment #672461 - Flags: feedback?(bent.mozilla) → feedback+
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.
(Assignee)

Comment 23

5 years ago
> 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.
Attachment #672461 - Flags: review?(anygregor) → review+
(Assignee)

Updated

5 years ago
status-firefox18: --- → affected
(Assignee)

Comment 25

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/56a682c947c4
(Assignee)

Updated

5 years ago
Whiteboard: [MemShrink:P1] → [MemShrink:P1][eventual-checkin-needed:aurora]
https://hg.mozilla.org/mozilla-central/rev/56a682c947c4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-aurora/rev/979bb0c18664
status-firefox18: affected → fixed
status-firefox19: --- → fixed
Whiteboard: [MemShrink:P1][eventual-checkin-needed:aurora] → [MemShrink:P1]
You need to log in before you can comment on or make changes to this bug.