Closed Bug 961952 Opened 6 years ago Closed 6 years ago

HardwareUtils.getMemSize can be expensive during startup

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: mfinkle, Assigned: mfinkle)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

HardwareUtils.getMemSize opens a file and reads the first 3 lines. It pulls the "MemTotal" from /proc/meminfo at startup. It's called via GeckoAppShell.getScreenDepth().

HardwareUtils.getMemSize accounts for 7.9% (186ms) of startup

Android has an internal method that does not use a RandomAccessFile reader. It might be faster:
http://androidxref.com/4.2_r1/xref/frameworks/base/core/java/com/android/internal/util/MemInfoReader.java
Follow-on from Bug 943073, which reduced the number of different implementations (and calls!) down to one.
Depends on: 943073
OS: Linux → Android
Hardware: x86_64 → All
Attached patch Faster getmem v0.1 (obsolete) — Splinter Review
This patch uses the technique found in the Android code:
* Load a block of the file into a buffer
* Scan for "MemTotal"
* Extract the value

The approach allows us to keep the buffer as a byte array and not convert it to a String as well.

Profiling shows the HardwareUtils.getMemSize dropped to 1.8% (43ms) of startup.
Assignee: nobody → mark.finkle
Attachment #8369661 - Flags: review?(rnewman)
Comment on attachment 8369661 [details] [diff] [review]
Faster getmem v0.1

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

::: mobile/android/base/util/HardwareUtils.java
@@ +101,5 @@
>          if (sTotalRAM >= 0) {
>              return sTotalRAM;
>          }
>  
> +        class MemInfoMatcher {

Can we get rid of this class and just make the methods private static on HardwareUtils class? It doesn't have any member variables...

Call the first method `matchMemInfoText`.

@@ +110,3 @@
>                  }
> +                for (int i=0; i<N; i++) {
> +                    if (buffer[index+i] != text.charAt(i)) {

Coding style! Spaces.

@@ +124,5 @@
> +                        while (index < buffer.length && buffer[index] >= '0'
> +                            && buffer[index] <= '9') {
> +                            index++;
> +                        }
> +                        String str = new String(buffer, 0, start, index - start);

Deprecated constructor. Specify an explicit ASCII Charset.

@@ +125,5 @@
> +                            && buffer[index] <= '9') {
> +                            index++;
> +                        }
> +                        String str = new String(buffer, 0, start, index - start);
> +                        return Integer.parseInt(str);

I like to see an explicit radix.

@@ +137,5 @@
> +        try {
> +            byte[] buffer = new byte[256];
> +            FileInputStream is = new FileInputStream("/proc/meminfo");
> +            is.read(buffer);
> +            is.close();

This `close` should be in a `finally` block around the `read`.

@@ +138,5 @@
> +            byte[] buffer = new byte[256];
> +            FileInputStream is = new FileInputStream("/proc/meminfo");
> +            is.read(buffer);
> +            is.close();
> +            String data = new String(buffer);

Explicit charset. And the behavior of this call when `buffer` is partly filled with 0s (or junk) is undefined, which is the case when /proc/meminfo returns fewer bytes than the buffer size.

Instead, take the return value of `read` and use the longer constructor argument to `String`.

@@ +141,5 @@
> +            is.close();
> +            String data = new String(buffer);
> +
> +            MemInfoMatcher m = new MemInfoMatcher();
> +            int length = buffer.length;

Nope. Use the return value of that initial `read` call. If you only read 170 bytes, you're going to be testing junk.

@@ +142,5 @@
> +            String data = new String(buffer);
> +
> +            MemInfoMatcher m = new MemInfoMatcher();
> +            int length = buffer.length;
> +            for (int i=0; i<length; i++) {

Spaces!

@@ +143,5 @@
> +
> +            MemInfoMatcher m = new MemInfoMatcher();
> +            int length = buffer.length;
> +            for (int i=0; i<length; i++) {
> +                if (m.matchText(buffer, i, "MemTotal")) {

Make matchText take two byte arrays, not a String. Do that translation work once (or statically!), and reduce the problem to byte array prefix testing.
Attachment #8369661 - Flags: review?(rnewman) → feedback+
(In reply to Richard Newman [:rnewman] from comment #3)

> > +        class MemInfoMatcher {
> 
> Can we get rid of this class and just make the methods private static on
> HardwareUtils class? It doesn't have any member variables...
> 
> Call the first method `matchMemInfoText`.

Done, but matchMemText which is consistent with the other function I lifted

> > +                for (int i=0; i<N; i++) {
> > +                    if (buffer[index+i] != text.charAt(i)) {
> 
> Coding style! Spaces.

Looks like Android coding uses my favorite style, or at least the sucker who wrote the file. Switched style.

> > +                        String str = new String(buffer, 0, start, index - start);
> 
> Deprecated constructor. Specify an explicit ASCII Charset.

Switched to the recommended constructor, which doesn't use a Charset, which should be fine given the file we are reading and the set of characters (all numbers).

> > +                        return Integer.parseInt(str);
> 
> I like to see an explicit radix.

Done.

> > +            is.close();
> 
> This `close` should be in a `finally` block around the `read`.


Done

> > +            String data = new String(buffer);
> 
> Explicit charset. And the behavior of this call when `buffer` is partly
> filled with 0s (or junk) is undefined, which is the case when /proc/meminfo
> returns fewer bytes than the buffer size.
> 
> Instead, take the return value of `read` and use the longer constructor
> argument to `String`.

It was left over cruft. I removed the line of code.

> > +            int length = buffer.length;
> 
> Nope. Use the return value of that initial `read` call. If you only read 170
> bytes, you're going to be testing junk.

Done

> > +            for (int i=0; i<length; i++) {
> > +                if (m.matchText(buffer, i, "MemTotal")) {
> 
> Make matchText take two byte arrays, not a String. Do that translation work
> once (or statically!), and reduce the problem to byte array prefix testing.

That is more than I care to do. My intentional was to port the existing code.
Attached patch faster-getmem v0.2 (obsolete) — Splinter Review
Updated
Attachment #8369661 - Attachment is obsolete: true
Attachment #8369817 - Flags: review?(rnewman)
Comment on attachment 8369817 [details] [diff] [review]
faster-getmem v0.2

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

::: mobile/android/base/util/HardwareUtils.java
@@ +100,5 @@
> +        if ((index + N) >= buffer.length) {
> +            return false;
> +        }
> +        for (int i = 0; i < N; i++) {
> +            if (buffer[index+i] != text.charAt(i)) {

One more...

@@ +107,5 @@
> +        }
> +        return true;
> +    }
> +
> +    private static int extractMemValue(byte[] buffer, int index) {

/**
 * @return the first uninterrupted sequence of digits following the specified index,
 *         parsed as an integer value in KB.
 */

@@ +112,5 @@
> +        while (index < buffer.length && buffer[index] != '\n') {
> +            if (buffer[index] >= '0' && buffer[index] <= '9') {
> +                int start = index;
> +                index++;
> +                while (index < buffer.length && buffer[index] >= '0' && buffer[index] <= '9') {

Missed this first time through: this function needs to take an upper bound, not just a start index.

Name those 'start' and 'end', perhaps. Pass in the read number of bytes.

@@ +127,2 @@
>       * Fetch the total memory of the device in MB by parsing /proc/meminfo.
>       * 

Fix trailing WS while we're here?

@@ -121,5 @@
> -                    return sTotalRAM = 0;
> -                }
> -
> -                // Parse a line like this:
> -                // MemTotal: 1605324 kB

Can we preserve this example somewhere?
Assignee: mark.finkle → rnewman
Status: NEW → ASSIGNED
Figured it'd be quicker for me to just address my own nits :D

r+ from me on this; if it meets with your approval, rock on.
Fixing assignee (bzexport sets it automatically).
Assignee: rnewman → mark.finkle
Attachment #8369832 - Flags: review+
Attachment #8369817 - Attachment is obsolete: true
Attachment #8369817 - Flags: review?(rnewman)
https://hg.mozilla.org/mozilla-central/rev/3d47addad575
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Comment on attachment 8369832 [details] [diff] [review]
HardwareUtils.getMemSize can be expensive during startup.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Extra time taken during startup
Testing completed (on m-c, etc.): It has been baking for a few days
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8369832 - Flags: approval-mozilla-aurora?
Attachment #8369832 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.