Closed Bug 863285 Opened 9 years ago Closed 9 years ago

Cache the result of Ci.nsIMemory.isLowMemoryPlatform()

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: tetsuharu, Assigned: tetsuharu)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch patch v1 (obsolete) — Splinter Review
This patch lazy loads Ci.nsIMemory as |Memory| in browser.js. But this name is very common. I seem we should name it |MemorySvc| or other good name. How do you think about it?
Attachment #739087 - Flags: review?(margaret.leibovic)
And, Can we cache the result value of Ci.nsIMemory.isLowMemoryPlatform()? It may be risky. However, from nsIMemory.idl (http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIMemory.idl#102), this method will return the same value over the lifetime of the process. If we cache the value, we reduce its function call in the "pageshow" event handler and the initializing tabs.
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #2)
> And, Can we cache the result value of Ci.nsIMemory.isLowMemoryPlatform()? It
> may be risky. However, from nsIMemory.idl
> (http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIMemory.idl#102),
> this method will return the same value over the lifetime of the process. If
> we cache the value, we reduce its function call in the "pageshow" event
> handler and the initializing tabs.

This is a good thought, but the implementation already caches the value in a static. We do not read the proc file on each call:
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsMemoryImpl.cpp#65
I also don't see the point of lazy-loading nsIMemory, because that code gets run unconditionally on startup anyway. With the other lazy-loading pieces, it is either potentially not run at all, or at least deferred from running on startup.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> I also don't see the point of lazy-loading nsIMemory, because that code gets
> run unconditionally on startup anyway. With the other lazy-loading pieces,
> it is either potentially not run at all, or at least deferred from running
> on startup.

But the current code calls |CC[].getService()| unconditionally for each times firing "pageshow" events and initializing tabs. This is very inefficient... 

I think lazy-loading is better than defining simply variable in this case. Or how about adding Ci.nsIMemory to Services.jsm?
Ah, in that case it might be better to just call isLowMemoryPlatform once at the beginning and cache that result. Then there's no need for the lazy loading code at all.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> Ah, in that case it might be better to just call isLowMemoryPlatform once at
> the beginning and cache that result. Then there's no need for the lazy
> loading code at all.

Oh, I see. I'll re-make the patch like so it.
Attached patch patch v2Splinter Review
Attachment #739087 - Attachment is obsolete: true
Attachment #739087 - Flags: review?(margaret.leibovic)
Attachment #739642 - Flags: review?(bugmail.mozilla)
Summary: Lazy load Ci.nsIMemory in browser.js → Cache the result of Ci.nsIMemory.isLowMemoryPlatform()
Comment on attachment 739642 [details] [diff] [review]
patch v2

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

This works, thanks. Did you by any chance measure what sort of impact this has?
Attachment #739642 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> This works, thanks. Did you by any chance measure what sort of impact this
> has?

No, I haven't yet. I pushed try-server just now: https://tbpl.mozilla.org/?tree=Try&rev=793379b9a8c6
https://hg.mozilla.org/mozilla-central/rev/eace0bc312cb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.