863285 - Cache the result of Ci.nsIMemory.isLowMemoryPlatform()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v1

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?

Comment 2

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

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.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 4

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

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.

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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?

Comment 6

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

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.

Comment 7

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 8

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v2

Comment 9

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

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?

Comment 10

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 11

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Try was green: https://tbpl.mozilla.org/?tree=Try&rev=793379b9a8c6

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eace0bc312cb

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eace0bc312cb