GeckoAppShell.isHighMemoryDevice and HardwareUtils.isLowMemoryPlatform are basically the exact same thing

RESOLVED FIXED in Firefox 28

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

({perf})

Trunk
Firefox 28
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: strictmode)

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
... and both of them synchronously read from /proc/meminfo on startup, causing strictmode violations.

These should be merged, so we only hit that once on startup.
(Assignee)

Updated

4 years ago
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 years ago
Also, we already have a nice implementation of fetching system memory in SysInfo, so both of these turn into two-liners with different thresholds.
(Assignee)

Comment 2

4 years ago
Created attachment 8339500 [details] [diff] [review]
Consolidate GeckoAppShell.isHighMemoryDevice and HardwareUtils.isLowMemoryPlatform.
(Assignee)

Comment 3

4 years ago
Comment on attachment 8339500 [details] [diff] [review]
Consolidate GeckoAppShell.isHighMemoryDevice and HardwareUtils.isLowMemoryPlatform.

I don't know why, but this actually eliminates *both* StrictMode warnings in my testing. And it now matches FHR. And deletes two different method implementations.

Hurrah!
Attachment #8339500 - Flags: review?(michael.l.comella)
(Assignee)

Comment 4

4 years ago
Note that I had to move SysInfo's implementation into HardwareUtils, because the latter comes earlier in the build than the former. It also makes sense there.
Comment on attachment 8339500 [details] [diff] [review]
Consolidate GeckoAppShell.isHighMemoryDevice and HardwareUtils.isLowMemoryPlatform.

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

I'm curious about these points, r+ if I like your answers. :)

::: mobile/android/base/util/HardwareUtils.java
@@ +90,5 @@
>      }
>  
> +    /**
> +     * Fetch the total memory of the device in MB by parsing /proc/meminfo.
> +     * 

nit: whitespace.

@@ +102,5 @@
> +            return sTotalRAM;
> +        }
> +
> +        try {
> +            RandomAccessFile reader = new RandomAccessFile("/proc/meminfo", "r");

Why not BufferedReader?

@@ +107,2 @@
>              try {
> +                // MemTotal will be one of the first three lines.

(For future travelers, see bug 872383 comment 8)

@@ +152,5 @@
> +        // Fallback to false if we fail to read meminfo
> +        // for some reason.
> +        if (memSize == 0) {
> +            Log.w(LOGTAG, "Could not compute system memory. Falling back to isLowMemoryPlatform = false.");
> +            return false;

Why false?
Attachment #8339500 - Flags: review?(michael.l.comella) → review-
(Assignee)

Comment 6

4 years ago
(In reply to Michael Comella (:mcomella) from comment #5)
 
> I'm curious about these points, r+ if I like your answers. :)

It's usual to just leave it set to r? if you have questions -- you don't have to change the flag each time you leave a comment. r- means "I have determined that this is broken; it must not land". Saves a bit of round-tripping.


> > +            RandomAccessFile reader = new RandomAccessFile("/proc/meminfo", "r");
> 
> Why not BufferedReader?

I don't remember. It probably has something to do with this not really being a real file (it's proc), so not caring about buffering, and not caring about character encodings here. BufferedReader adds both of those things.

Feel free to file a separate bug if you think changing this method might be worthwhile, and someone can try benchmarking and testing it on different devices.

More broadly: I opted to stick with the implementation that has been returning millions of values to FHR from various platforms, rather than the implementations that might have been quietly failing on some devices! All three pieces of code are old.


> Why false?

Because that was the original behavior; I didn't even change the comment. I wasn't here to redefine what we consider to be a low-memory platform.
(In reply to Richard Newman [:rnewman] from comment #6)
> > Why false?
> 
> Because that was the original behavior; I didn't even change the comment. I
> wasn't here to redefine what we consider to be a low-memory platform.

Sorry, I mean why do we choose to assume that this device is not a low-memory platform when it fails? For FHR purposes?
Comment on attachment 8339500 [details] [diff] [review]
Consolidate GeckoAppShell.isHighMemoryDevice and HardwareUtils.isLowMemoryPlatform.

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

Since this info is being collected by FHR, it makes sense that we don't want to change the definition of low-memory devices so this is good with the old implementation.
Attachment #8339500 - Flags: review- → review+
(Assignee)

Comment 9

4 years ago
(In reply to Michael Comella (:mcomella) from comment #7)

> Sorry, I mean why do we choose to assume that this device is not a
> low-memory platform when it fails? For FHR purposes?

Taking a step back:

There are three things in the tree.

* SysInfo.getMemSize, which parses /proc/meminfo to find out how many MB of RAM the phone has. This is used by FHR.

* HardwareUtils.isLowMemoryPlatform, which parses /proc/meminfo to find out if the phone has less than 384MB of RAM. This is used by the HomePager to decide whether to show the Reading List.

* GeckoAppShell.isHighMemoryDevice, which parses /proc/meminfo to find out if the phone has more than 768MB of RAM. This is used to decide whether to use 24-bit color.


What this patch does is reimplement isLowMemoryPlatform and isHighMemory in terms of getMemSize, so we have *one* place in the tree that parses /proc/meminfo.

The *logic* of isLowMemoryPlatform and isHighMemoryDevice remain the same -- isLowMemoryPlatform continues to return false on failure, and so does isHighMemoryDevice.

That is, a device whose memory size cannot be read is assumed to be neither especially big nor especially small -- somewhere in the middle. Those devices will get 16-bit color and Reading List.

It's nothing to do with FHR. FHR reports the value of getMemSize directly. What I'm preserving are the thresholds for Reading List or 24-bit color enablement. This is a 100% refactor patch.

Thanks for the review!
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/07a264873fa5
Target Milestone: --- → Firefox 28
https://hg.mozilla.org/mozilla-central/rev/07a264873fa5
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Blocks: 961952
You need to log in before you can comment on or make changes to this bug.