Closed Bug 532039 Opened 10 years ago Closed 10 years ago

Improve perf of IsLowMemory

Categories

(Core :: XPCOM, defect, critical)

All
Maemo
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta5-fixed

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(1 file, 1 obsolete file)

On average, 0.418 seconds of startup of Fennec on my N900 is spent in IsLowMemory.  We can do better.


I attempted to memory map the high watermark file in, but it isn't supported.  This patch will cache the file descriptor.  In doing so, we reduced the startup cost caused by IsLowMemory to 0.082 seconds (about 5x faster.)


I have verified, via TestOOM, that we continue to get the the notification when we run out of memory.

The only real issue I have with this patch is that we are not cleaning up the file descriptor.
Attached patch patch v.1 (obsolete) — Splinter Review
Assignee: nobody → mozbugz
Attachment #415344 - Flags: review?(benjamin)
tracking-fennec: --- → ?
Attached patch patch v.2Splinter Review
don't leak
Attachment #415344 - Attachment is obsolete: true
Attachment #415399 - Flags: review?(benjamin)
Attachment #415344 - Flags: review?(benjamin)
Comment on attachment 415399 [details] [diff] [review]
patch v.2

>diff --git a/xpcom/base/nsMemoryImpl.cpp b/xpcom/base/nsMemoryImpl.cpp

> #elif defined(NS_OSSO)
>-    int fd = open (kHighMark, O_RDONLY);
>-    if (fd == -1) {
>-        *result = PR_FALSE;
>-        return NS_OK;
>+
>+    static AutoFDClose osso_highmark_fd;

static AutoFDClose really doesn't make a lot of sense here... the OS is going to close this file descriptor for us when the process exits: why not just do `static int osso_highmark_fd = -1` here?

>+    if (osso_highmark_fd == -1) {
>+        osso_highmark_fd = open (kHighMark, O_RDONLY);

The declaration of kHighMark at the top of the file is incorrect: it should be

static const char kHighMark[] = "...";

instead of

const char* kHighMark = "...";

Currently the pointer is non-const, which makes things less efficient and potentially confusing.

r=me with comments fixed
Attachment #415399 - Flags: review?(benjamin) → review+
mfinkle made me feel guitly last night for not closing the file descriptor.  the last patch (v.1) basically didn't include this auto close class.  I will use that instead, and fix the declaration.
Attachment #415399 - Flags: approval1.9.2+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/81eaca7acbb2
http://hg.mozilla.org/mozilla-central/rev/410468c50ca1
Status: NEW → RESOLVED
tracking-fennec: ? → ---
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 540545
You need to log in before you can comment on or make changes to this bug.