801818 - Add an API to determine the available memory budget

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Various pieces of code (such as graphics tiling/displayport) can adjust their behaviour dynamically based on how much memory is available. We should set up a proper API for these pieces of code to find out how much memory they have available to them so that they can tune their behaviour.

AFAIK the only memory-dependent behaviour we have is limited to (1) setting prefs at compile-time using the MOZ_PKG_SPECIAL define which really just distinguishes between ARMv7 and ARMv6, and (2) throwing away things when we receive low-memory notifications from the system. This bug would add a third behaviour where we prevent allocating memory if we know the platform doesn't have a lot.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

An API similar to the one I had in mind used to exist and was removed, see bug 592308. However, in the case of android, the free memory reported as reported by /proc/meminfo is "malleable" in that android can shut down other activities to increase free memory. Therefore for our purposes the MemTotal value would be more useful. Perhaps it is worth implementing the nsIMemory.isLowMemory() just for android, and having the implementation just cache the MemTotal value.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Add isLowMemoryPlatform API

What do you guys think about adding an API that looks like this?

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 673598 [details] [diff] [review]
Add isLowMemoryPlatform API

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

::: xpcom/base/nsMemoryImpl.cpp
@@ +68,5 @@
> +    if (sLowMemory == -1) {
> +        FILE* fd = fopen("/proc/meminfo", "r");
> +        if (!fd) {
> +            return NS_ERROR_FAILURE;
> +        }

I'm not sure we want to keep retrying to open meminfo on every call if it fails. Perhaps we should set sLowMemory to the default value before trying to open meminfo.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add isLowMemoryPlatform() to nsIMemory

Updated with some cleanup and to default it to false if the file operations fail. The first caller will still get back an NS_ERROR_FAILURE, but subsequent callers will get NS_OK.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Expose isLowMemoryPlatform via AndroidBridge

AFAICT there's no way to get a hold of an nsIMemory instance in script, so I added a readonly boolean to AndroidBridge to expose the new API. Verified it works as expected from browser.js

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 674748 [details] [diff] [review]
Part 2 - Expose isLowMemoryPlatform via AndroidBridge

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

nsIMemory is already scriptable. I'd rather we set up the factory constructor to make it a service than to proxy it through nsIAndroidBridge

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 674748 [details] [diff] [review]
Part 2 - Expose isLowMemoryPlatform via AndroidBridge

Oh, turns out I screwed up when I first tried accessing this via JS. Getting a handle to it from Cc["@mozilla.org/memory;1"].getService(Ci.nsIMemory) does work after all, so this second patch is unnecessary.

Comment 8

[Benjamin Smedberg]

Comment on attachment 674744 [details] [diff] [review]
Part 1 - Add isLowMemoryPlatform() to nsIMemory

I don't think this method should ever fail. Please make the current NS_ERROR_FAILURE cases just fall through and always succeed. r=me with that change. I'm happy to rereview if you're not certain of the conditionals.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Updated to make the error cases always return NS_OK and return "false" for the result of the check.

https://hg.mozilla.org/integration/mozilla-inbound/rev/653f15147169

Comment 10

[Justin Lebar (not reading bugmail)]

If you remember, can you please cc me on bugs which use this API?  If you guys are doing anything interesting in those bugs, we should probably flip this switch for B2G as well.

Comment 11

[Justin Lebar (not reading bugmail)]

Oh, duh, they're dependent on this bug.  Ignore me.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

(In reply to Justin Lebar [:jlebar] from comment #11)
> Oh, duh, they're dependent on this bug.  Ignore me.

The ones I have so far are dependent on this bug, yes. I'll try to CC you on any future ones which I may or may not mark dependent on this.

In terms of "flipping the switch" I think the only thing that would need to be done is expand the #ifdef block to include b2g.

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/653f15147169