Closed Bug 971728 Opened 6 years ago Closed 6 years ago

Use memory-pressure events instead of minimization when sending an application into the background

Categories

(Firefox OS Graveyard :: General, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed)

RESOLVED FIXED
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

Details

(Keywords: perf, Whiteboard: [c=memory p= s= u=tarako])

Attachments

(1 file)

While reading the issue in bug 961348 it occurred to me that whenever we launch a new application another one gets sent into the background (typically the homescreen); this triggers a minimization of the newly backgrounded application in order to save memory. See the code here:

http://hg.mozilla.org/mozilla-central/file/879038dcacb7/dom/ipc/ProcessPriorityManager.cpp#l1037

This is problematic: memory minimization is a very expensive procedure which issues three memory-pressure events and thus runs the GC/CC three times over. This procedure is long enough that it can interfere with the starting application and it also touches all the heap memory thrice making it detrimental for something like zRAM. In addition to this it could introduce glitches during fast app switching which forced us to introduce a procedure to cancel it in flight in bug 814771 which added a fair bit of complexity to the process.

Today I experimented with changing this logic to send a single memory-pressure event to the backgrounded process instead of a full minimize. I've then measured the resulting effect this had on memory consumption in a few tests (all done with heavy reference workloads). For every test I measured how much memory was saved by the memory minimization procedure and how much by a simple memory-pressure event. The tests were the following:

1) I've opened the dialer, went into the call log, went into contacts list, waited for it to settle then sent it into the background.

2) I've opened the SMS app, went into the big SMS thread, waited for it to settle then sent it into the background.

3) I've opened the Calendar app, swiped three times back then created an event, then sent it into the background.

These are the results I got which were fairly consistent:

+------+--------------+-----------------+
| test | minimization | memory-pressure |
+------+--------------+-----------------+
|    1 |      4.2 MiB |         4.1 MiB |
|    2 |      1.1 MiB |         1.1 MiB |
|    3 |      0.6 MiB |         0.6 MiB |
+------+--------------+-----------------+

I can do more testing but it seems to me that there's really no point in using a minimization over a memory-pressure event. From a CPU usage perspective minimization takes a fair bit of extra CPU usage for very little or no benefit.

Here's two profiles of the SMS app going into the background, first with minimization, >120ms are spent in the GC (look for the last CPU spike to the right):

http://people.mozilla.org/~bgirard/cleopatra/#report=9bc3e816fc0162c61bb1e25ae791b0a3efc74510

Then with a memory-pressure event, only ~50ms are spent in the GC:

http://people.mozilla.org/~bgirard/cleopatra/#report=de1173d7986d8c2fac58fa9df0ce64830200fdb0

I'm cooking up a patch to change this and I suggest testing this on a Tarako device to see if it has a positive impact on application startup (I don't have one, otherwise I would do it myself).
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Nice!  Let's get this into triage today.
blocking-b2g: --- → 1.3T?
traige: will give this patch a try on tarako before deciding. Thanks Fabrice
Flags: needinfo?(fabrice)
Keywords: perf
Priority: -- → P2
Whiteboard: [c=memory p= s= u=tarako]
I tested this patch on top of the current 1.3t branch. That seems to help a little with startup, but it's clearly not a huge gain. That's low risk though so I'm fine with taking it.
Flags: needinfo?(fabrice)
Comment on attachment 8374849 [details] [diff] [review]
[PATCH] Use memory-pressure events instead of minimization when sending an app to the background

(In reply to Fabrice Desré [:fabrice] from comment #4)
> I tested this patch on top of the current 1.3t branch. That seems to help a
> little with startup, but it's clearly not a huge gain. That's low risk
> though so I'm fine with taking it.

Excellent, let's go then. This is a green try run with the patch applied:

https://tbpl.mozilla.org/?tree=Try&rev=7cf7656c6dcd
Attachment #8374849 - Flags: review?(khuey)
Comment on attachment 8374849 [details] [diff] [review]
[PATCH] Use memory-pressure events instead of minimization when sending an app to the background

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

r=me
Attachment #8374849 - Flags: review?(khuey) → review+
Does this patch conflict with bug 963477?
Flags: needinfo?(pwang)
I don't think so.
Flags: needinfo?(pwang)
(In reply to Thinker Li [:sinker] from comment #7)
> Does this patch conflict with bug 963477?

Definitely not, in fact it should mitigate the issue that caused bug 963477. Without this patch we were running three GC/CC cycles in a row when sending an app to the background, with this patch applied it goes down to only on GC/CC cycle. I'd suggest redoing the tests in bug 963477 with this patch applied and GC re-enabled to see how this affects startup time and memory consumption.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3d18224c5065
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 975434
traige: 1.3T+ to get this into tarako
blocking-b2g: 1.3T? → 1.3T+
Depends on: 989847
You need to log in before you can comment on or make changes to this bug.