Closed Bug 801818 Opened 8 years ago Closed 8 years ago

Add an API to determine the available memory budget

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 19

People

(Reporter: kats, Assigned: kats)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
See Also: → 592308
Blocks: 803122
Attached patch Add isLowMemoryPlatform API (obsolete) — Splinter Review
What do you guys think about adding an API that looks like this?
Attachment #673598 - Flags: feedback?(blassey.bugs)
Attachment #673598 - Flags: feedback?(benjamin)
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.
Attachment #673598 - Flags: feedback?(blassey.bugs) → feedback+
Attachment #673598 - Flags: feedback?(benjamin) → feedback+
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.
Assignee: nobody → bugmail.mozilla
Attachment #673598 - Attachment is obsolete: true
Attachment #674744 - Flags: review?(blassey.bugs)
Attachment #674744 - Flags: review?(benjamin)
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
Attachment #674748 - Flags: review?(blassey.bugs)
Attachment #674744 - Flags: review?(blassey.bugs) → review+
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
Attachment #674748 - Flags: review?(blassey.bugs) → review-
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.
Attachment #674748 - Attachment is obsolete: true
Blocks: 805909
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.
Attachment #674744 - Flags: review?(benjamin) → review+
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
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.
Oh, duh, they're dependent on this bug.  Ignore me.
(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.
https://hg.mozilla.org/mozilla-central/rev/653f15147169
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
You need to log in before you can comment on or make changes to this bug.